Python - Konwersja CSV na obiekty - Code Design

Question

Mam mały skrypt, którego używamy do odczytu w pliku CSV zawierającym pracowników i wykonujemy podstawowe manipulacje na tych danych.

Odczytujemy dane (import_gd_dump) i tworzymy obiekt Employees zawierający listę obiektów Employee (może powinienem wymyślić lepszą konwencję nazewnictwa ... lol). Następnie wywołujemy clean_all_phone_numbers() na Employees, która wywołuje clean_phone_number() na każdym Employee, a także lookup_all_supervisors(), na Employees.

import csv 
import re 
import sys 

#class CSVLoader: 
# """Virtual class to assist with loading in CSV files.""" 
# def import_gd_dump(self, input_file='Gp Directory 20100331 original.csv'): 
#  gd_extract = csv.DictReader(open(input_file), dialect='excel') 
#  employees = [] 
#  for row in gd_extract: 
#   curr_employee = Employee(row) 
#   employees.append(curr_employee) 
#  return employees 
# #self.employees = {row['dbdirid']:row for row in gd_extract} 

# Previously, this was inside a (virtual) class called "CSVLoader". 
# However, according to here (http://tomayko.com/writings/the-static-method-thing) - the idiomatic way of doing this in Python is not with a class-function but with a module-level function 
def import_gd_dump(input_file='Gp Directory 20100331 original.csv'): 
    """Return a list ('employee') of dict objects, taken from a Group Directory CSV file.""" 
    gd_extract = csv.DictReader(open(input_file), dialect='excel') 
    employees = [] 
    for row in gd_extract: 
     employees.append(row) 
    return employees 

def write_gd_formatted(employees_dict, output_file="gd_formatted.csv"): 
    """Read in an Employees() object, and write out each Employee() inside this to a CSV file""" 
    gd_output_fieldnames = ('hrid', 'mail', 'givenName', 'sn', 'dbcostcenter', 'dbdirid', 'hrreportsto', 'PHFull', 'PHFull_message', 'SupervisorEmail', 'SupervisorFirstName', 'SupervisorSurname') 
    try: 
     gd_formatted = csv.DictWriter(open(output_file, 'w', newline=''), fieldnames=gd_output_fieldnames, extrasaction='ignore', dialect='excel') 
    except IOError: 
     print('Unable to open file, IO error (Is it locked?)') 
     sys.exit(1) 

    headers = {n:n for n in gd_output_fieldnames} 
    gd_formatted.writerow(headers) 
    for employee in employees_dict.employee_list: 
     # We're using the employee object's inbuilt __dict__ attribute - hmm, is this good practice? 
     gd_formatted.writerow(employee.__dict__) 

class Employee: 
    """An Employee in the system, with employee attributes (name, email, cost-centre etc.)""" 
    def __init__(self, employee_attributes): 
     """We use the Employee constructor to convert a dictionary into instance attributes.""" 
     for k, v in employee_attributes.items(): 
      setattr(self, k, v) 

    def clean_phone_number(self): 
     """Perform some rudimentary checks and corrections, to make sure numbers are in the right format. 
     Numbers should be in the form 0XYYYYYYYY, where X is the area code, and Y is the local number.""" 
     if self.telephoneNumber is None or self.telephoneNumber == '': 
      return '', 'Missing phone number.' 
     else: 
      standard_format = re.compile(r'^\+(?P<intl_prefix>\d{2})\((?P<area_code>\d)\)(?P<local_first_half>\d{4})-(?P<local_second_half>\d{4})') 
      extra_zero = re.compile(r'^\+(?P<intl_prefix>\d{2})\(0(?P<area_code>\d)\)(?P<local_first_half>\d{4})-(?P<local_second_half>\d{4})') 
      missing_hyphen = re.compile(r'^\+(?P<intl_prefix>\d{2})\(0(?P<area_code>\d)\)(?P<local_first_half>\d{4})(?P<local_second_half>\d{4})') 
      if standard_format.search(self.telephoneNumber): 
       result = standard_format.search(self.telephoneNumber) 
       return '0' + result.group('area_code') + result.group('local_first_half') + result.group('local_second_half'), '' 
      elif extra_zero.search(self.telephoneNumber): 
       result = extra_zero.search(self.telephoneNumber) 
       return '0' + result.group('area_code') + result.group('local_first_half') + result.group('local_second_half'), 'Extra zero in area code - ask user to remediate. ' 
      elif missing_hyphen.search(self.telephoneNumber): 
       result = missing_hyphen.search(self.telephoneNumber) 
       return '0' + result.group('area_code') + result.group('local_first_half') + result.group('local_second_half'), 'Missing hyphen in local component - ask user to remediate. ' 
      else: 
       return '', "Number didn't match recognised format. Original text is: " + self.telephoneNumber 

class Employees: 
    def __init__(self, import_list): 
     self.employee_list = []  
     for employee in import_list: 
      self.employee_list.append(Employee(employee)) 

    def clean_all_phone_numbers(self): 
     for employee in self.employee_list: 
      #Should we just set this directly in Employee.clean_phone_number() instead? 
      employee.PHFull, employee.PHFull_message = employee.clean_phone_number() 

    # Hmm, the search is O(n^2) - there's probably a better way of doing this search? 
    def lookup_all_supervisors(self): 
     for employee in self.employee_list: 
      if employee.hrreportsto is not None and employee.hrreportsto != '': 
       for supervisor in self.employee_list: 
        if supervisor.hrid == employee.hrreportsto: 
         (employee.SupervisorEmail, employee.SupervisorFirstName, employee.SupervisorSurname) = supervisor.mail, supervisor.givenName, supervisor.sn 
         break 
       else: 
        (employee.SupervisorEmail, employee.SupervisorFirstName, employee.SupervisorSurname) = ('Supervisor not found.', 'Supervisor not found.', 'Supervisor not found.') 
      else: 
       (employee.SupervisorEmail, employee.SupervisorFirstName, employee.SupervisorSurname) = ('Supervisor not set.', 'Supervisor not set.', 'Supervisor not set.') 

    #Is thre a more pythonic way of doing this? 
    def print_employees(self): 
     for employee in self.employee_list: 
      print(employee.__dict__) 

if __name__ == '__main__': 
    db_employees = Employees(import_gd_dump()) 
    db_employees.clean_all_phone_numbers() 
    db_employees.lookup_all_supervisors() 
    #db_employees.print_employees() 
    write_gd_formatted(db_employees) 

Po pierwsze, moje pytanie brzmi preambuła, widać nic złego z powyższym, zarówno z projektu klasy lub Python point-of-view? Czy dźwięk logiki/projektu?

Tak czy inaczej, do specyfiki:

1. Przedmiotem Employees ma metodę, clean_all_phone_numbers(), która wywołuje clean_phone_number() na każdym Employee obiektu wewnątrz niego. Czy to zły projekt? Jeśli tak, dlaczego? Czy sposób, w jaki dzwonię, jest zły?
2. Oryginalnie owinąłam metodę clean_phone_number() i lookup_supervisor() w jednej funkcji, z pojedynczą pętlą for wewnątrz. clean_phone_number to O (n), jak sądzę, lookup_supervisor to O (n^2) - czy to jest w porządku, dzieląc go na dwie pętle?
3. W clean_all_phone_numbers() zapętlałem obiekty Employee i ustawiam ich wartości za pomocą polecenia return/assignment - czy powinienem ustawić to samo w sobie, clean_phone_number()?

Jest też kilka rzeczy, które zostały w jakiś sposób zhackowane, nie wiem, czy to zła praktyka - np. print_employee() i gd_formatted() oba używają __dict__, a konstruktor dla Employee używa setattr() do konwersji słownika na atrybuty instancji.

Ja ceniłbym w ogóle wszystkie myśli. Jeśli sądzisz, że pytania są zbyt szerokie, daj mi znać, a ja mogę je odświeżać, ponieważ kilka osób się rozdzieliło (po prostu nie chciałem zanieczyszczać tablic wieloma podobnymi pytaniami, a te trzy pytania są mniej lub bardziej ściśle powiązane).

Cheers, Victor

Answer 1

wygląda dobrze do mnie. Dobra robota. Jak często będziesz uruchamiać ten skrypt? Większość twoich pytań jest dyskusyjna, jeśli jest to kwestia jednorazowa.

1. Lubię sposób Employees.cleen_all_phone_numbers() delegatów Employee.clean_phone_number()
2. Naprawdę należy przy użyciu indeksu (słownik) tutaj. Możesz indeksować każdego pracownika przez hrid, kiedy tworzysz je w O(n), a następnie sprawdzasz je w O(1).
   • Ale tylko to zrobić, jeśli kiedykolwiek musiał ponownie uruchomić skrypt ...
   • Wystarczy dostać się do nawyku korzystania ze słowników. Są bezbolesne i sprawiają, że kod jest łatwiejszy do odczytania. Ilekroć piszesz metodę lookup_*, prawdopodobnie chcesz tylko indeksować słownik.
3. Nie jestem pewien. Lubię jawnie ustalać stan, ale jest to w rzeczywistości zły projekt - powinien to zrobić pracownik, który powinien być odpowiedzialny za swój stan.

Answer 2

należy zamknąć swoje pliki po przeczytaniu ich Proponuję przeniesienie wszystkich skompilowane Re tot he najwyższym poziomie (w przeciwnym razie skompilować je każde połączenie) jeśli self.telephoneNumber jest brak lub self.telephoneNumber == „”: cen być łatwo przerobiony, jak gdyby nie self.telephoneNumber