קוד מעולה יהיה פשוט (ולכן לרוב קצר) יעיל ומסביר את עצמו ללא צורך בתיעוד.
זה לא קוד מעולה
לא בטוח אם כל אחד, לכל משימה, יכול להפיק קוד מעולה.
עדיין, הקוד כנראה טוב ולא מסובך במיוחד, כך שהוספת תיעוד (שבמסגרת תרגיל זה לא עשיתי) יכולה להשלים את ולעזור לקוד להיות קריא וברור.
במיוחד כשהקורא לא יודע את מטרת התכנית בכללותה ומטרות הפונקציות בפרט, קשה יותר לקוד לבדו “להסביר את עצמו”. יש סיכוי שגם אתה קצת התרחקת מהמטרות של החלקים השונים - לדוגמה הצעת עכשיו:
לבצע handle לפקודה בתוך get_clerk כשהמקום הראוי לזה (כפי שאתה הצעת שתי תגובות קודם) הוא בלולאה הראשית.
לבדוק אם פקיד “עסוק” - קונספט לא רלוונטי באופן ישיר, כשבאופן עקיף רלוונטי רק לפקודות מסוימות.
אני רואה את מטרת פונקציית get_clerk במחלקת Dispatcher כך:
לקבל את מאפייני הפקודה לטיפול ולהחזיר אובייקט של פקיד המתאים לטפל בה.
מאפייני הפקודה הם: הפעולה לביצוע, שם הפקיד המבוקש, שם לקוח (רלוונטי רק לפעולת “המתנה”).
לכן לוגיקת הפונקציה הזו היא:
אם הפקיד המבוקש עדיין לא במאגר הפקידים של ה-Dispatcher, צור וצרף אתו.
אם הפעולה לביצוע דורשת שיהיו לפקיד לקוחות בתור, קבל שם של פקיד כזה (הפקיד המבוקש אם יש לו לקוחות, אחרת פקיד אחר עם לקוחות אם יש כזה)
החזר את אובייקט הפקיד
שיפור מסוים לקריאות של הקוד עשוי להיות אם אשתמש ב CommandTools.customer_actions כחלק מהתנאי בזיהוי סוג הפעולה לביצוע. אבל אם יודעים את מטרת הפונקציה ומבנה האובייקטים Dispatcher ו-Clerk הקוד די ברור לדעתי.
def get_clerk(self, cmd_atr):
clerk_on_call = cmd_atr['clerk']
self.add_clerk_if_not_exist(clerk_on_call)
if cmd_atr['action'] in CommandTools.customer_actions:
clerk_on_call = self.get_clerk_with_customers(clerk_on_call)
return self._clerks[clerk_on_call]
הפונקציה הבאה שהייתי ניגש לטפל בה היא הפונקציה parse_command של CommandTools.
שים לב איך הפונקציה הזאת צריכה להכיר את כל סוגי הפקודות וכמה פרמטרים כל פקודה צריכה לקבל ובדיוק איפה כל פרמטר שייך לכל פקודה. זה המון לוגיקה שככל שהשפה תאפשר יותר פקודות הפונקציה הזאת הולכת לגדול ולהיות קשה לתחזוקה.
בפיתוח מונחה עצמים אפשר להתמודד עם מצבים כאלה באמצעות חלוקה למחלקות - נסה להגדיר לכל סוג פקודה מחלקה משלה, לשנות את parse_command כך שהיא רק תצטרך לזהות איזה סוג פקודה היא קיבלה, ותתן למחלקת הפקודה המתאימה לשבור את הראש איך לפענח את כל הפרמטרים. זה יבטל לך את כל ה if-ים בפונקציה ואני חושב שיעשה אותה קצת יותר פשוטה
אני לא חושב שהגישות שלנו שונות ומתחבר מאוד לדוגמה סדרן המוניות.
אולי ההבדל נובע מהסתכלות שלך על הפונקציה הספציפית במנותק מהמחלקה שלה ומכלל התכנית:
למחלקת “סדרן המוניות” שלנו יש “מאגר” פקידים ב-dict עם שמות הפקידים כמפתחות המצביעים על האובייקט שלהם.
בסוף הפונקציה פשוט מחזירים את האובייקט לפי מפתח שם הפקיד. ללא “חיפוש”, אלא בעזרת שם הפקיד שהתקבל ממאפייני הפקודה שניתנו לנו (או ממקרה מיוחד, אליו אתייחס בסעיף הבא).
לכן הפעולה הראשונה בפונקציה היא רק לייצר ולהוסיף את הפקיד אם הוא לא קיים. לא “לחפש” ולהחזיר כבר בשלב זה את האובייקט של הפקיד.
בהגדרת הבונוס של התרגיל התייחסת לפקודת next כך שיטופל לקוח מתור של פקיד אחר אם אין לקוחות לפקיד שבפקודה.
הלוגיקה הנובעת מזה היא: בפקודת next; אם הפקיד המבוקש פנוי (אין לקוחות בתור); יש להשתמש בפקיד עם לקוחות.
לכן יצרתי את get_clerk_with_customers שמופעלת אם מדובר על פקודת next והיא “דואגת” לזה - תחזיר לנו שם של פקיד בהתאם ללוגיקה הזו (כולל הפקיד “המקורי” אם יש לו לקוחות בתור).
[בפקודה get_clerk_with_customers יש שימוש בפקודה המקבילה ל-is_busy (שהיא has_customers_in_queue).]
לכאורה אמורות להיות להם מטודות זהות מבחינת ממשק, כשבפרק 24 המלצת להשתמש בירושה במקרים כאלו, למרות שבשימוש הנוכחי (המאוד מצומצם) באמת לא נראה שיהיה בזה תועלת (אלא רק אם יהיה משהו שבאמת אפשר לשתף בינהן).
from collections import deque
class Clerk:
def __init__(self, clerk_name):
self.name = clerk_name
self._queue = deque()
def handle(self, cmd_atr):
if 'wait' == cmd_atr['action']:
self.wait(cmd_atr['customer'])
elif 'next' == cmd_atr['action']:
self.next()
def wait(self, customer):
self._queue.append(customer)
def next(self):
try:
print(self._queue.popleft())
except IndexError:
print('There are no more customers to serve.')
def has_customers_in_queue(self):
return True if 1 <= len(self._queue) else False
class CommandTools:
valid_actions = ('wait', 'next')
customer_actions = ('wait')
no_valid_command_msg = """
Please use one of these formats:
wait <clerk-name> <customer-name>
next <clerk-name>
"""
@staticmethod
def read_command():
while True:
cmd_input = input(': ')
try:
return CommandTools.parse_command(cmd_input)
except UserWarning as e:
print(f'No valid command was given: {e}', CommandTools.no_valid_command_msg)
continue
@staticmethod
def parse_command(command):
command_words = command.split(' ')
# if command_words[0] not in CommandTools.valid_actions:
# raise UserWarning(f'Invalid command action ({command_words[0]}).')
try:
if 'wait' == command_words[0]:
return WaitCommand.parse(command)
elif 'next' == command_words[0]:
return NextCommand.parse(command)
else:
raise UserWarning(f'Invalid command action ({command_words[0]}).')
except ValueError:
raise UserWarning(f"Invalid number of words for {command_words[0]} action.")
class WaitCommand:
@staticmethod
def parse(command):
com_atr = {}
(com_atr['action'], com_atr['clerk'], com_atr['customer']) = command.split(' ')
return com_atr
class NextCommand:
@staticmethod
def parse(command):
com_atr = {}
(com_atr['action'], com_atr['clerk']) = command.split(' ')
return com_atr
class Dispatcher:
def __init__(self):
self._clerks = {}
def get_clerk(self, cmd_atr):
clerk_on_call = cmd_atr['clerk']
self.add_clerk_if_not_exist(clerk_on_call)
if 'next' == cmd_atr['action']:
clerk_on_call = self.get_clerk_with_customers(clerk_on_call)
return self._clerks[clerk_on_call]
def add_clerk_if_not_exist(self, name):
if name not in self._clerks:
clerk_obj = Clerk(name)
self._clerks[name] = clerk_obj
def get_clerk_with_customers(self, requested_clerk):
if self._clerks[requested_clerk].has_customers_in_queue():
return requested_clerk
for c in self._clerks:
if c != requested_clerk and self._clerks[c].has_customers_in_queue():
return c
return requested_clerk
def print_queues(self):
for c in self._clerks:
print(f'{c}\t{self._clerks[c]._queue}')
dispatcher = Dispatcher()
while True:
cmd = CommandTools.read_command()
clerk = dispatcher.get_clerk(cmd)
clerk.handle(cmd)
נראה הרבה יותר טוב. ירושה בפייתון רלוונטי רק למקרים בהם צריך לשתף קוד (וגם אז צריך לזכור שזה לא המנגנון היחיד ולחשוב איזה מנגנון הכי מתאים למקרה שלך). פה אין קוד משותף אז כמו שאמרת זה לא בסיפור.
רק טיפ קטן בגלל שאנחנו בפייתון אפשר להפוך את הפונקציה parse_command אפילו לעוד יותר פשוטה ועם פחות if-ים בצורה כזאת:
class Summer:
def __init__(self):
self.total = 0
def add(self, *numbers):
self.total += sum( i for i in numbers )
def print_total(self):
print(self.total)
s = Summer()
t = Summer()
s.add(10, 20)
t.add(50)
s.add(30)
# should print 60
s.print_total()
# should print 50
t.print_total()
Targil 2
class MyCounter:
count = 0
def __init__(self):
MyCounter.count += 1
for _ in range(10):
c1 = MyCounter()
# should print 10
print(MyCounter.count)
פקודת sum מקבלת רשימה (למעשה, כל סוג של iterable) ויצירת הלולאה הפנימית (List comprehensions) מיותרת - היא בעצם יוצרת בדיוק את אותה רשימה.
אז אפשר להסתפק ב:
הי יש פה משהו קצת מבלבל - מצד אחד האוביקט Clerks שומר כשדה מידע פנימי את clerks שזו רשימה של כל הפקידים. מצד שני כשהוא צריך למצוא פקיד פנוי הוא משתמש במשתנה בשם listOfClerks שבכלל מוגדר קצת יותר למטה.
החלק שמדאיג אותי בדרך כלל בתרגילים הללו הוא עד כמה אני בודק ומכין את המעטפת (בדיקת נכונות קלט, try-except וכד’…) - הפעם למדתי ששווה להכין פונקציות ו/או class’ים “שעושים את העבודה” ולטפל בנפרד בנכונות הקלט
שאלה קטנה. מחלקות גם מקובל לכתוב רק באותיות קטנות או שמחלקות כן מתחילות באות גדולה?
מצורף תרגיל 4 שלי אשמח לקבל הערות.
class Clerk:
def __init__(self):
self.queue = []
def wait(self, name):
self.queue.append(name)
def next(self):
if len(self.queue) > 0:
return self.queue.pop(0)
class Queue:
def __init__(self):
self.clerks = {
'dave': Clerk(),
'michael': Clerk(),
}
def wait(self, clerk, name):
self.clerks[clerk].wait(name)
def next(self, name):
name = self.clerks[name].next()
i = 0
for o in self.clerks.values():
name = o.next()
if name:
print(name)
return
def get_clerk(self, clerk):
if clerk not in self.clerks:
self.clerks[clerk] = Clerk()
return clerk
class Command:
error_glob = "the input must begin with : and get next / wait after it"
error_params = "you must give name of clerk and for wait must also give name for customer"
def __init__(self):
self.queue = Queue()
def input_line(self):
while True:
_in = input()
while not _in[0] == ":":
if _in == "exit":
return True
print(self.error_glob)
_in = input()
try:
_, func, clerk, *name = _in.split()
except ValueError:
print(self.error_params)
continue
if func not in ["next", "wait"]:
print(self.error_glob)
continue
clerk = self.queue.get_clerk(clerk)
if func == "next":
if name:
print(self.error_params)
continue
self.queue.next(clerk)
else:
if not name:
print(self.error_params)
continue
self.queue.wait(clerk, name[0])
c = Command()
while True:
if c.input_line():
break
