Code Review למוות

[S h a r k 1 8]

Code Review למוות

במקום העבודה הנוכחי שלי על כל pull request עושים כמה וכמה סבבים של code review. בד"כ הסבב הראשון משפר את איכות הקוד, אבל אח"כ אני לפחות מרגיש שמתעסקים בקטנות ובשטויות והתוספות/שינויים שמכניסים לקוד במסגרת הסבבים המאוחרים יותר לא באמת הופכים את הקוד ליעיל/קריא יותר, אלא זה יותר טעם אישי או סגנון אישי של ה reviewer. כבר היו לי מקרים שבסבבים מאוחרים אמרו לי שהייתי צריך אחרת, וה"אחרת" הזה זה קוד שכבר כתבתי ושינו לי אותו במסגרת סבב code review מוקדם יותר.

זה קצת מתסכל אותי כי אני מרגיש שאני מבזבז המון זמן על כלום.

רציתי לשאול אתכם איך זה בעבודה שלכם. כמה זמן משקיעים ב code review? על אילו דברים מעירים? האם רק על דברים שהם clear cut או גם על דברים שיש בהם טעם אישי?

 

[Lhuna1]

בעיה ידועה

קודם כל לעשות כמה סבבים על אותו קוד זה בזבוז זמן וחוסר יעילות משווע.
(פרט למקרים יוצאי דופן שהקוד המקורי באמת השתנה משמעותית).
&nbsp
אבל גם בסבב אחד הבעיה תמיד קיימת, כי לצערי הרבה אנשים לא מבינים את המטרה של קוד ריוויו, וכמו שאמרת חושבים שהמטרה היא לשכתב את הקוד שיהיה יותר לטעמם, כמו שהם היו כותבים אותו, ונטפלים לקטנות ולשטויות שאינן בעיות אמיתיות אלא ענייני סגנון.
&nbsp
אצלנו כרגע לצערי דווקא אין מספיק קוד ריוויו (כלומר הרבה קוד נכנס בלי שום ריוויו), אבל מצד שני עבדתי במקומות שהיה הרבה יותר מידי קוד ריוויו מיותר ולא יעיל, ובינינו, אני לא יודעת מה עדיף.
&nbsp
בעקרון לא אמורים להעיר על עניינים שהם טעם אישי.
אבל לך תשכנע מתכנתים מסויימים שמתייחסים לשטויות כמו אל דת, שמה שהם חושבים שהוא אמת מוחלטת זה רק הטעם האישי שלהם. בהצלחה.
&nbsp

 

[user32]

 

[selalerer]

נשמע יותר מידיי

אצלינו עושים code review אחד.
&nbsp
הרעיון שמישהו נוסף יעבור על הקוד הוא מצויין. עוד זוג עיניים תופס דברים שאני לא ראיתי, מעלה שאלות חשובות שלא שאלתי את עצמי כשכתבתי ועוזר לשפר את האיכות.
&nbsp
ככל שיש יותר סבבים (שלא רק בודקים שתוקן מה שדיברנו עליו קודם) הסיכוי שבאמת יעזרו לשפר משהו יורד (כנראה אקספוננציאלית).
&nbsp
תמיד יש איזשהו מתח בין איכות לבין יעילות עבודה. ברוב המקומות שמפתחים תוכנה אין איזה צורך קיצוני באיכות. כמובן שזה דבר טוב ורצוי אבל גם מוצר שלא מתקדם מילימטר מגרסה לגרסה זה רע מאוד.
&nbsp
אולי למישהו שהוא ממש עובד חדש יש טעם בכמה סבבים, אבל גם זה עדיף שיהיה מול מתכנת מנוסה אחד (לכל pull request) ולא כל פעם לקבל הערות ממישהו נוסף שיכולות לפעמים להיות סותרות.
&nbsp
אני חושב גם שהרבה מתכנתים (גם מנוסים) חושבים קצת יותר מידיי במונחים של שחור ולבן ומסמנים דברים כשגויים על אף שהם לא בהכרח כך, ולא פותחים דברים לדיון ושיקולים של ייתרונות וחסרונות

 

[fellow1]

יש דבר כזה?

למזלי גם בעבודה הקודמת וגם בזו רק הבוס או מתכנת בכיר בודקים לי את הקוד פעם אחת ומעירים הערות וזהו.

מה שטוב בלכתוב דרייברים זה שאתה צריך ללמוד על החומרה ולקרוא הרבה חומר. לא תמיד מישהו חיצוני יכול לבדוק את מה שאתה כותב ישירות בלי להשקיע שעות בעצמו והם לא נלהבים לעשות זאת.

לפי מה שראיתי מתכנתים ברמות הגבוהות יותר באמת סובלים יותר מהבחינה הזאת, אבל זה ממש לא נורא בהשוואה לעבודה מול חומרה.

 

[ipv6]

כל אחד מגדיר אחרת

הערות שהן "clear cut" והערות שהן טעם אישי ותמיד אפשר לטעון לשני הכיוונים.
קח את ההערות CR הכי "קטנוניות" בעינך שקבלת, מתכנת סביר ידע להסביר למה הוא ביקש שתתקן את זה ולא יגיד "סתם כי אני אוהב שהקוד נראה ככה".

יצא לי לעבוד תחת בוסים שאהבו לעשות review דקדקני שיורד לרמות של שמות משתנים, הערות ועד התפלפלויות על perofrmance במקומות שלא מעניינים אף אחד. אחד הבוסים לשעבר היה מבקש לתקן רקבון שנתקלת בו.
קיבלת פיצ'ר קטן באזור שלא כתוב טוב, יכולת לקבל הערת CR לשכתב אזור כלשהוא מהאזור הרקוב, שהעלות של השיכתוב הזה פי 3 מעלות פיתוח הפיצ'ר. הייתי גם במצב כזה.

זה לא בהכרח רע והיו אנשים שהרגישו שהם למדו מ-CR כאלה ולא התלוננו כל עוד היה לזה את הגיבוי מההנהלה (כלומר הבינו שלאותו בוס יש דרישות גבוהות ופיצ'רים קטנים לכאורה עשויים לקחת זמן).

הבעיה עם CR דקדקני מגיעה כשההנהלה לא יודעת לתמחר נכון את העלות שגוזל מהמפתח שצריך לעמוד בסטנדרט גבוה.

 

[vinney]

code review זה כלי מצוין

אם משתמשים בו נכון... נשמע שאצלך לא בדיוק משתמשים בו נכון.
&nbsp
המטרה בזה זה לא למצוא באגים (קשה למצוא באגים מסתם קריאה של קוד אלא אם זה משהו לחלוטין טריויאלי ומאוד בולט). המטרה זה לאכוף את הcoding conventions כדי שקוד של כולם יראה דומה וייכתב בצורה אחידה. כך יהיה יותר קל למישהו להכנס לקוד של מישהו אחר אם צריך לעשות תיקונים/שינויים. זה גם כלי לא רע ללמוד את השפה/טכנולוגיה - מישהו יותר מנוסה יציע לפתור את הבעיה בדרכים שאתה אולי בכלל לא מכיר ולא חשבת עליהן.
&nbsp
הגיוני שכל הסבבים יתנהלו עם אותם הreviewers, וכך המצב הזה שאתה מתאר שמישהו בסבב מאוחר דורש ממך את ההיפך של מה שדרשו בסבב הקודם יימנע.
&nbsp
כדי שזה יהיה יעיל כדאי גם שיהיו coding standards מוגדרים והחלטה ברמת הצוות/מחלקה של מה חשוב וצריך לאכוף ומה לא חשוב ויישאר לטעם האישי של כל מתכנת (למשל - חשוב לסדר פרמטרים כך שקלט יהיה קודם ופלט אחר כך, אבל לא חשוב לשים {} בif שנכתב בשורה אחת. או להיפך).

 

[choo]

קוד רוויו לא נועד רק לכתיבה אחידה של סינטקס

&nbsp
הוא נועד גם לוודא שהקוד יהיה דבאגבילי (כולל הוספה של הערות, הדפסות ללוג, מונים חיצוניים וכדומה), לוודא שהקוד ברור לעוד קורא בלי קשר לסטנדרטים, לוודא שטופלו מקרי קצה רלונטיים (לרוב זה לא דורש להיכנס עמוק ללוגיקה), לוודא שתוכניות הבדיקה הורחבו לכסות את הקוד החדש על שלל מקרים מעניינים, וכשמדובר במפתח חדש או בקוד רגיש במיוחד - בהחלט גם למצוא באגים.

 

[vinney]

אלה לרוב הדברים שמכוסים בcoding standards

איך כותבים לוגים? איך סופרים דברים תוך כדי ריצה? איך כותבים טסטים?
&nbsp
אלה בדיוק הדברים שאמורים להיות מכוסים בcoding standards או guidelines או conventions או איך שלא קוראים לזה.
&nbsp
מי שלא מכיר יכול לבוא ולדבג עם printfים - יבוא הreviewer ויסביר איך עושים את זה ויפנה למקום בו זה מפורט. מי שלא מכיר לא יכתוב טסטים - יעירו לו ויצביעו לו למקום בו זה מפורט.
&nbsp
מקרי קצה זה סוג של באג, ואם כותבים טסטים כמו שצריך - אז הם יהיו מכוסים. אז כן, ישימו על זה דגש בריוויו, ובקטעים רגישים במיוחד מישהו יכנס גם לנבכי הלוגיקה. אבל בגדול - קוד ריוויו לא נועד לחיפוש באגים, ולא אמורים למצוא בו באגים. אם מוצאים באגים בריוויו זה אומר שהמתכנת פישל לגמרי בטסטים או לא הבין מה הוא אמור לעשות.

 

[choo]

אני אוהב את הקביעות הגורפות הללו

&nbsp
"אם יש באג שהתגלה ברוויו - המפתח שכתב את הקוד פישל" זו הכללה גורפת מדי. אנשים עושים טעויות, וייתכן שמפתח לא חשב על מקרה קצה מסוים ולכן גם לא בדק אותו וזה לא נקרא "לפשל לגמרי", וזה קורה. לגבי סטנדרט של קוד - אי אפשר לכתוב קונוונציות שמכסות כל מקרה, אחרת מקבלים מסמך שבפועל לא יתייחסו אליו כי לא ניתן יהיה לזכור אותו (לרוע המזל הזכרון של אנשים מוגבל ביכולותיו, אפילו אם הם מפתחי תוכנה טובים), ובניגוד לדברים כמו "איפה פותחים סוגריים" או "איך מסדרים רווחים" - את הקונוונציות הכלליות הללו אי אפשר למצוא אוטומטית באמצעות תוכנה פשוטה, כי צריך להפעיל מידה כלשהי של שיקול דעת.

 

[vinney]

לא הבנתי מה ניסית להגיד...

אם יש באג בקוד - המפתח פישל, לרוב. אם יש באג בקוד שלא נתגלה עד הריוויו - המפתח פישל מאוד.
&nbsp
כמובן במקומות לא מסודרים בהם אין תרבות של בדיקות והבטחת איכות, ריוויו זה המקום היחיד בו הבאגים יתגלו לפני שהם יגיעו לפרודקשן, אבל פה שוב - הבעיה במקרה הזה זה לא תהליך הריוויו.
&nbsp
לא צריך לזכור הרבה מהקונבנציות האלה - יש כלים אוטומטיים שיפרמטו לך את הקוד לפי הקונבנציות שהגדירו, יש כלים אוטומטיים שיבדקו כיסוי טסטסים לפי הפרמטרים שהגדירו, ובסוף כשאתה מגיע לריוויו אחרי הכלים האלה הריוויו יכלול את הקריאות של הקוד והאם הטסטים כתובים בצורה נכונה ומסודרת, ואמורים להסתיים אחרי סבב, גג שניים.

 

[choo]

הבנתי, אז כמעט כל המפתחים מפשלים, כי יש להם באגים

&nbsp
אפשר לפטר את אנשי הבדיקות ופשוט לדרוש מהמפתחים "לא לפשל"?
&nbsp
באגים מתקיימים כי אנחנו כבני אדם לא מושלמים, ולא תמיד מסוגלים לחשוב על כל המקרים.
באגים מתקיימים בגלל צורך לשחרר מוצרים לשוק מהר שמובילים לעיגול פינות בצד הטכנולוגי, בידיעה שזה יותר יעיל לחברה באותו זמן מבחינה עסקית.
בדיקות אוטומטיות וידניות של הקוד לא יובילו למציאת כל הבאגים.
מפתח שכותב את הבדיקות לקוד של עצמו יעשה באגים שעליהם הוא לא חשב (ולכן לא חשב על בדיקות ספציפיות עבורם).
&nbsp
הגישה של "אם יש באג המפתח פישל" היא לא גישה פרודוקטיבית.
&nbsp
לגבי בדיקה אוטומטית של כיסוי - לרוב מדובר בכיסוי של שורות או פונקציות, ולא בכיסוי של מצבים, ולכן עדיין יש צורך במחשבה אנושית כדי לוודא שמצבים חשובים (אי אפשר באמת לכסות את כולם מעבר למחלקות מאוד פשוטות שאינן משתמשות במחלקות אחרות) מכוסים על ידי הבדיקות.
&nbsp
נ.ב. בין הרוויו לפרודקשין יש בדיקות QA פונקציונאליות מערכתיות, בדיקות עומסים, בדיקות עמידות בתקלות (HA) וכיוצא בזה. יש לבאגים עוד מקומות להתגלות בהם אחרי הרוויו.
&nbsp
וכן - יהיו גם באגים אצל לקוחות. טרם נתקלתי בחברת תוכנה שהצליחה להימנע מבאגים אצל לקוחות. זה משהו שחשוב להכיר בו, ולהכין כלים ופרוצדורות שיאפשרו להתמודד עם תוצאותיהם בשטח.

 

[vinney]

וודאי שכולם מפשלים

זה נראה שלקחת את ה״פשלה״ כמשהו שלילי.
&nbsp
לגבי בדיקות אוטומטיות של כיסוי - אתה צודק, לרוב מדובר בכיסוי זמן ריצה, לא כיסוי מצבים, אבל אם עוקבים אחרי מדדים אחרים גם (למשל שמירה על LOC נמוך פר מתודה, cyclomatic complexity נמוך, וכד), כיסוי זמן ריצה מתחיל להתקרב מאוד לכיסוי מצבים. את כל המדדים האלה אפשר לבדוק בצורה אוטומטית.
&nbsp
לגבי בדיקות QA - בדיקות כאלה מבוצעות ברמות גבוהות הרבה יותר מבדיקות יחידה ויש סיכוי לא קטן שבאג ברמת היחידה לא יתגלה בQA כי יהיה מוסתר על ידי הגנות ביחידות אחרות בצורה כזאת או אחרת. הבאג הזה פתאום יתגלה שנתיים אחרי כשמישהו ישנה משהו ביחידה אחרת לגמרי כך שההגנה שהיחידה הזאת סיפקה פתאום נעלמה לך.
&nbsp
אז אם כל מה שהקפיץ אותך זה המילה ״פשלה״, אז תנסה לקרוא אותה בכל צורה אחרת שתעשה לך נעים בגב, אבל מעבר לזה לא הבאת שום טיעון חדש נוסף למה שכבר אמרתי בעצמי.

 

[hadooper]

.

כשאני סוקר, אני פשוט עובר על כל השינויים top to bottom.

אני מתעכב בעיקר על דברים פונקציונאליים ופחות קוסמטיים בקוד, אבל בסוף מעיר על כל דבר שנראה לי בעייתי, מנסה להסביר באופן מדוייק איך הייתי עושה זאת אחרת ואף דורש עליו סקירה חוזרת לאחר התיקון. לרוב אין צורך ביותר מסבב אחד אפקטיבי, אבל זה הרבה בגלל שכיום הכימיה עם אנשי הצוות שלי יחסית טובה.

 

[יבגניי34]

אפשר ורצוי להחליף חלק ניכר מה״דעות״ בניתוח קוד סטטי.

אם מדברים ב code review על דברים כמו איפה הסוגריים - זו תקלה תהליכית.

 

[liron50]

תלוי באופי מקום העבודה שלך

אם זה מקום מסודר שפתוח לקבל ביקורת - תגיד למי שצריך שיש חוסר אחידות בצורת הבדיקות שייבדקו איך לייעל את התהליך.
אפשר לכתוב מסמך שמגדיר מה צריך להיבדק במסגרת ה-codereview וכן מסמך עם code standard שהמתכתנתים צריכים לעבוד בהתאם אליו.
במקום מסוים שהייתי בו, לפני שמישהו היה רשאי לעשות code review הוא היה צריך לקבל לזה הסמכה. ממש היה צריך ללמוד ולהיבחן לגבי מה בודקים ומה לא + מבחן מעשי. אפשר לומר שזה מוגזם, אבל יש לזה תוצאות (ועלויות)..
&nbsp
אם אתה במקום לא כל כך מסודר שבו דברי ביקורות יורדים באסלה, אז תצטרך להחליט או להתיישר לפי מה שאומרים לך או לצפצף על מה שאומרים ולראות לאן זה יוביל.
בהרבה מקרים כשה-code reviewer רואה שלא מתייחסים להערות שלו הוא מתגמש. הרבה פעמים כשיש לחץ ורוצים לסגור דברים המנהלים מאיצים בו ומבקשים שאם יש דברים לא קריטים שירשום אותם בצד.
+לפעמים גם קורה שהוא רוצה להחזיר לך המשימה לתיקונים אלא שאתה כבר עסוק בדברים אחרים, ואז מעבירים את המשימה אליו. ומאחר שזה די מתסכל- לתקן שטויות שמישהו אחר כתב -הוא לומד לפעמים הבאות להיות פחות ביקורתי.

 

[Lhuna1]

רעיון ההסמכה נשמע לי לא רע, טרם נתקלתי בו

כמובן אם מבצעים אותו טוב.
לא מוגזם בעיני, כי העלות של קוד ריוויו'ז שנעשים לא נכון יכולה להיות מאוד גבוהה.
&nbsp

 

[offWhiteRider]

מאוד מאוד

א ק ס פ פ רי ס