אני לא יודע האם אפשר בכלל לנסח מדריך כזה, ואני יודע שכל אחד עושה את ה-code review קצת אחרת.
אתאר את הגישה שלי ואשמח לתגובות, הערות ותוספות.
1. ראשית, כותב הקוד צריך לעשות review עצמי, לנקות ולסדר אותו כדי שיהיה קריא בצורה בסיסית. זאת גם ההזדמנות לגלות שאריות שהן commented-out, פונקציות שלא משתמשים בהן, שורות ארוכות ולא קריאות, תיעוד חסר, שגיאות כתיב ודקדוק בהערות וכד'.
2. יש שתי גישות לביצוע ה-code review. הגישה הראשונה היא גישת ה-offline ובה השינוי המוצע נשלח במייל או מועלה לאתר ייעודי שבו הוא נגיש למפתחים אחרים והם יכולים לכתוב הערות (במייל או באתר עצמו). הייתרון בגישה הזאת הוא בכך שה-review מתבצע בזמן אחר שנוח ל-reviewer ובמקרה של אתר, מאפשר גם שיתוף של כמה אנשים בצורה נוחה יחסית (חבר המליץ לי על תוכנת ה-reviewboard במקרה כזה). אני חושב שה-review יכול להיות אפקטיבי בשיטה כזאת אבל לא בכל המקרים. במקרה שנדרש דיון יסודי יותר ולא רק הערות מסוג "שים לב שהפונקציה יכולה לזרוק exception במקרה כזה וכזה" השיטה העדיפה היא קריאה משותפת של הקוד וסיכום ההערות על נייר או במייל.
שיטה כזאת של "קריאה משותפת" מתאימה מאוד למפתחים חדשים (בפרוייקט הספציפי ובכלל), אבל גם במקרים אחרים היא הזדמנות לחזור ולבדוק את הדרישות, ה-design המפורט ולקיים על כך דיון קצר. החיסרון בשיטה כזאת הוא חוסר היכולת לשתף כמה reviewers. (אני יודע שיש מקומות שבהם עושים ישיבות review רבות משתתפים וקוראים את הקוד מהמקרן, אבל אני חושב שיש בעיות רבות בשיטה: א' - מוזמנים בד"כ גם אנשים שאין להם הרבה קשר לקוד ויקח רק שעה להסביר להם במה מדובר, ב' - מאותה הסיבה קל לדיון לסטות לכיוונים אחרים ולא קשורים בכלל, ג' - לא כל האנשים אוהבים את המעמד הזה של מעין "משפט שדה" שבו קלונם יכול לצאת ברבים בגלל שגיאה טפשית)
3. מי עושה את ה-review? אם מדובר בשינוי קוד קיים, חייבים לשתף את כותב הקוד המקורי ואם אינו שותף לפרויקט יותר, מישהו שקיבל את האחריות עליו. במקרה של חלק מהמודול - רצוי לשתף את המתכנן של אותו מודול ואת מי שמימש חלקים גדולים ממנו. כך אפשר לעלות על שימוש לא נכון ב-APIs, סטייה מ-invariants או הכנסת state למערכת שצריכה להיות stateless, רחמנא ליצלן. אפשר לעשות את ה-review גם על ידי סמכות מקצועית בקבוצה. אני חושב שלא כדאי לעשות את ה-code review עם מי שיש לו רק הסמכות הניהולית (יכול לקרות בניהול מטריציוני). רצוי גם לדאוג שכל אנשי הפרויקט יתנסו בלעבור code review ובלעשות code review לאחרים. לא צריכים להיות כאן "מיוחסים".
4. טוב, אז מה בודקים?
א. ראשית כל קריאות. לדעתי הדבר החשוב ביותר שצריך לבדוק הוא קריאות הקוד. אחרת, יבוא המפתח הבא, יגיד "מי כתב את השטות הזאת" וישכנע את כולם לשכתב את הפרויקט מאפס. גם אותו מפתח שכתב את הקוד ייתכן שלא יזכור אחרי חצי שנה מה הוא כתב ולמה. במקרה כזה, כאשר הסיבות למימוש בצורה א' ולא בצורה ב' לא ממש ברורות, המערכת הופכת ל"קופסה שחורה", וכאשר יש באגים ושינויים, מטפלים בסימפטומים שלהם ולא בבעיה עצמה, כי את הבעיה קשה מאוד להבין. אני לא רוצה לפרט את כל הגורמים ההופכים קוד לקריא או להיפך (נכתבו על כך ספרים ובלוגים רבים).
ב. design. כן, זה נשמע קצת מאוחר לבדוק את ה-design בסוף הפיתוח, אבל מה לעשות, לפעמים חלק מהשינויים הנדרשים ב-design מתגלים רק תוך כדי עבודה, דרישות חדשות נכנסות אף הן באמצע הפיתוח, ומערכת קטנה וחמודה שיכלה לזכות בפרס ה-design הבינלאומי מתגלה כדומה יותר למפלצת מלוך נס. נקודת ה-review היא הזמן לשלוח את מפלצת לסדנת טיפוח שבמהלכה אפשר לזהות design patterns ולממש אותם כמו שצריך, להוציא חלק מהפונקציונליות למודול נפרד, ואיפה שאין ברירה, לתחום בבירור את הקוד ה"מכוער" בתקווה שאפשר יהיה לעשות איתו משהו בעתיד.
ג. בדיקות. האם יש לקוד בדיקות (רצוי unit tests) שבודקות את הכתוב?
ד. התאמה לקונבנציות. זה אמנם קשור קצת לקריאות, אבל מקומות שונים מפתחים סט של קונבנציות מקומיות (כמו האם לשים I לפני שם של interface או לא)
ה. טיפול במקרי קצה. עד כמה שנתאמץ לא נוכל לפתח בדיקות שבודקות כל היבט בפעולת המערכת בכל מצב אפשרי. לא את כל ה-exceptions אפשר להכניס לסט הבדיקות, כי למשל לא תמיד אפשר לנתק תקשורת TCP באמצע. אם לא נטפל במקרים אלה עכשיו כנראה שהפעם הבאה אחרי ה-review שנסתכל על הקוד הזה תהיה כאשר הלקוחות יתחילו להתקשר למרכז התמיכה. לכן ה-code review הוא הזמן להיזכר ולפעמים גם להחליט איך אנחנו רוצים שהמערכת תתנהג במצבי שגיאה שלא תוכננו מראש. באותה הזדמנות כדאי לבדוק איך המערכת מתמודדת ומודיעה על שגיאות (למשל עדיף להודיע למתשמש שאין לו הרשאות כתיבה בדיסק מאשר לזרוק exception עם הטקסט "this exception should never be thrown")
ו. נכונות. כן, שמתי את ה-item הזה אחרון, כי מעבר לתכניות פשוטות או שגיאות גסות ובולטות, אני לא חושב שב-review אפשר להתמודד עם בעיות של נכונות. גם בגלל שהזמן שהושקע בפיתוח הושקע בעיקר בנכונות, ובגלל שבדקנו שיש בדיקות (סעיף ג') לעיל, אני חושב שבעיות ב"נכונות" יתגלו במהרה ואם אינן מהותיות הן קלות יחסית לתיקון. אני יודע שיש כאן גם דיעות אחרות.
5. כמה זמן מקצים ל-code review? מהנסיון האישי שלי, למדתי שצריך להקצות זמן ראוי ל-code review, שנע איפשהו באיזור ה-20% מזמן הפיתוח בפועל, והוא כולל גם את הזמן לשינויים. כמובן שזמן זה משתנה מסיטואציה אחת לשניה, אבל חשוב להקצות אותו מראש ולא לאכול את הזמני ה-buffer שהוקצו לפרויקט לצורך ה-review.
ומכאן, אשמח לשמוע את דעתכם!
הוספת תגובה על "המדריך הלא שלם ל-code review"
נא להתחבר כדי להגיב.
התחברות או הרשמה