למה לא כדאי להכריח Code-Reviews?

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

מקור: Unsplash

מאת: רוני שפירא

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

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

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

Code Reviews לא נועדו לעלות על באגים

אחת הטענות הרווחות היא שמציאת באגים בפרודקשן יקרה פי 5-10 יותר מאשר במהלך הפיתוח, ומכיוון ואפילו השינוי הקטן ביותר עלול ליצור באג, שווה לעשות Review גם על השינויים הקטנים ביותר.

אז נכון, אם במהלך ה-Review מוצאים איזשהו באג זה מעולה, אבל מעבר לעובדה שלא סביר לצפות מה-Reviewer ״להריץ״ את הקוד בראש ולצפות מה יקרה בכל אחד ממקרי הקצה, Code Reviews כלל לא נועדו לשרת את המטרה הזו, וזו תהיה טעות להתייחס ל-Code Review כאמצעי שיטתי למניעת באגים.

מחקרים רבים נעשו על הנושא, ובעוד שכולם מסכימים ש-Code Reviews חשובים מאוד, אין הסכמה גורפת לגבי האפקטיביות שלהם במציאת באגים. מחקר של מיקרוסופט מצא שרק 15% מההערות ב-Reviews קשורות למציאת באגים, וגם אלו התייחסו לרוב לבעיות מינוריות בלבד.

אם באגים זה מה שמטריד אתכם, ישנן דרכים יעילות יותר למצוא אותם :

– ביצוע Design Reviews לפני כתיבת קוד יעזור למפתחים לוודא שהפתרון שלהם עונה על הדרישות

– כלי של Penetration Test יעזור למצוא בעיות אבטחה

– מתודולוגיית (TDD) Test Driven Development ובדיקת אחוזי כיסוי תבטיח שלכל קוד חדש יש טסטים

– כלי Static Code analysis יכולים לסייע בבדיקות קוד טריוויאליות כמו משתנים לא מאותחלים, קוד לא נגיש, אכיפת קוד סטייל וכו׳.

אמון

אתם סומכים על המפתחות והמפתחים בחברה שלכם? אני מקווה שכן.

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

במידה וכך זה גם אצלכם, עולה השאלה – למה דווקא כשזה נוגע ל-Code Review אומרים "אני רוצה שעל כל דבר קטן שנעשה תעבור עין נוספת"? איזה מסר זה מעביר לצוות? האם זה תורם לחיזוק האמון בסביבת העבודה?

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

זה יאט את הקצב שלכם (הרבה פעמים, בלי סיבה טובה)

בואו נניח לרגע שהצוות פיתח פיצ'ר חדש ומגניב. מדובר בשינוי משמעותי, ולכן נעשה Code Review מקיף ולאחר כמה הערות ותיקונים הלוך-חזור, השינויים מאושרים והגיע רגע ה-merge הנכסף. Woohoo!

זמן קצר לאחר מכן הצוות קיבל בקשה לעשות שינוי קטן בעיצוב – "סליחה, כנראה פספסנו משהו, הכותרת הזו קצת גדולה מדי, היא H2 אבל צריכה להיות H3, אפשר לשנות את זה?". "אין בעיה" אתם אומרים, וחושבים לעצמכם ״רק לשנות את ה-headingType מ-H2 ל-H3, בדיוק 4 שניות של עבודה״.

האם שינוי כזה *באמת* צריך Code Review נוסף? הרי הפיצ׳ר כבר נבדק, הלוגיקה טובה, הטסטים ירוקים וכל הצוות מכיר את הקוד. איזה ערך Review כזה ייצור מלבד להאט אתכם וליצור תסכול?

הצד של ה-Reviewer

פינג! סלאק מתריע – pull request חדש. אי אפשר להתעלם, נכון? בלי אישור אי-אפשר למרג׳ג׳.
אז נכון, כשיש מדיניות שצריך לבצע Review על כל שינוי הצוות יתאמץ לבצע אותם במהירות כדי לא לעכב את התהליך, אבל האם כל Review יתבצע ביסודיות הנדרשת?

אנחנו טוענים שאם ה-Review הוא אופציונלי, Reviewers שידעו שביקשו ספציפית את הפידבק שלהם ירגישו אחריות יתרה להתעמק, להבין היטב את הקוד ולספק פידבק איכותי יותר. זאת בניגוד למצב שבו ה-Reviewer התבקש/ה לעבור על הקוד כי ״זו המדיניות״ או כי ״צריך אישור זריז כדי להתקדם״.

כשעושים Review על הכל, ללא אבחנה, כל הזמן, Reviewers עלולים להיכנס לשגרה בה כבר קשה לזהות את אותם שינויים מורכבים ומסוכנים שבאמת צריכים תשומת לב גבוהה. Code Review fatigue זה דבר אמיתי!

אחריות ולמידה

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

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

באופן דומה אנחנו חושבים גם על Code Review. מפתחים בוחרים אם הם מעוניינים ב-Review על העבודה שלהם או לא, והם מצופים לעמוד מאחורי הבחירות שלהם, ובעיקר – ללמוד מהן. למשל, אם משהו נשבר בפרודקשן, הצוות יעשה post mortem, ינתח מה השתבש ובעיקר יחפש הזדמנויות להשתפר כדי לוודא שתקלה דומה לא תחזור בעתיד.

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

כדי לספק לצוות בטחון להכניס קוד לפרודקשן גם בלי Code Review, השקענו משאבים רבים במערכות ניטור והתראות שיעזרו לנו לגלות בעיות בזמן אמת, בהטמעת מערכת לניהול feature flags המאפשרת שחרור מדורג של פיצ׳רים ללקוחות ספציפיים, תהליכי CI/CD, אימוץ מתודולוגיית trunk based development הדוגלת בביצוע שינויים קטנים ותכופים במקום branch-ים ענקיים שקשה לעשות להם Review וכמובן – טסטים אוטומטיים, מכל הסוגים, והרבה מהם.

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

Code Reviews זה חשוב, מאוד

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

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

כיום, מרבית הצוותים מבקשים Code Reviews על כ-70% מהשינויים שלהם, ולקראת הרבעון הבא הצבנו יעד להגדיל את המספר הזה, עם זאת, כנראה שלעולם לא נכוון ל-100%.

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

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

אצלנו למשל, לא ניתן להכניס שינויים ללא אישור ל-repository של הקומפוננטות המשותפות שלנו (design system). זאת מכיוון וכל הצוותים משתמשים בה ויש חשיבות יתרה שכל קומפוננטה תהיה בדוקה ומתועדת היטב. כמו כן, מכיוון שצוותים רבים כותבים קוד ב-repository הזה, יותר קשה לשמור על אחידות וסטנדרטים.

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

הכותב הוא VP Engineering בחברת הסטארטאפ Duda

כתב אורח

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

הגב

12 תגובות על "למה לא כדאי להכריח Code-Reviews?"

avatar
Photo and Image Files
 
 
 
Audio and Video Files
 
 
 
Other File Types
 
 
 

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

סידור לפי:   חדש | ישן | הכי מדורגים
יוניטי מן
Guest

ממש סטארטאפ ניישן. חוסכים אוברהד במקום אחד כדי לקבל אותו במקום אחר.
שיפט לפט מישהו?

מרק ק.
Guest
חברה אחת שיודעים מראש שהקוד שלה שבור ולא מתוחזק ורצוי להתחמק מלעבוד בה אלא אם מבטיחים לכם שתכתבו קוד מאפס. כן בטח TTD, ומי בודק שאכן הקוד בטסטים אכן בודק את כל אפשרויות הקלט הרלבנטיות ולא רק יוצא ידי חובה בבדיקת מסלולי ביצוע קוד מקסימליים? בטח, אם מעסיקים מתכנתים מעולים זה לא כל כך משנה באיזו מטודולוגיה משתמשים, הבעיה היא שלרוב מעסיקים גם מתכנתים פחות מעולים, שגם למתכנתים מעולים יש ימים שהם לא מרןכזים מסיבה כזו אן אחרת, ושבבסיס קוד גדול, גם עבור מתכנתים מעולים יש אזורים בקוד שבהם הם לא מתמצאים. עושה רושם לפי המאמר הזה ש הVP לא… Read more »
אשר
Guest

מאמר ממש מביך.
קוד ריוויו זה אומר שלא סומכים על המתכנתים?

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

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

יוניטי מן
Guest

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

סווטה
Guest

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

עמיר
Guest

Peer programming זו גם דרך להגביר את הבטיחות של הקוד.

רון
Guest

אוי, duda.. איזו חברה גרועה..
הייתי אצלם בראיון, איזה שאלות הזויות ויחס הזוי היה

נילס
Guest

חברה מעולה!

סתם, האמת שאין לי מושג

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

נילס
Guest

מגיע גוניור דוחף לפרודקשן, מפיל
מגיע יום שני, דוחף נופל..

מחקר של מיקרוסופט? אולי זה מסביר את טריליון הבאגים שיש בכל מוצר ומערכת הפעלה שלהם

אחד שמבין אבטחה
Guest
אחד שמבין אבטחה

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

ג.ב.
Guest

This is called a 'shift right' methodology because your shifting the potential bugs, performance issues and vulnerabilities right into production

מפרגן
Guest
אני מאוד מעריך את הכותב על הפתיחות המחשבתית, ההליכה נגד הזרם והאומץ- זה ממש לא טריוויאלי לקרוא תיגר על מתודולוגיה נפוצה כ"כ ואני חושב שאנחנו צריכים לעודד כאלה מאמרים. היה ברור לי שיהיו מגיבים שינצלו את ההזדמנות לתקוף באגרסיביות ובכך לבסס (בעיני עצמם) את מעמדם כמתכנתי על שבאבחת תגובה אחת ילמדו את כל העולם איך צריך לחשוב. אבל לעניין- אני לא מסכים. אמנם זה נכון שיש לפעמים קוד רביוז לשינויים טריוויאליים אבל: 1. לתת למפתח להחליט אם צריך רביו או לא זה לא כ"כ פשוט- במקרי הקצה זה כמובן כן פשוט אבל תמיד יהיו שטחים אפורים. 2. לפעמים טעות של… Read more »
wpDiscuz

תגיות לכתבה: