ללמוד מהקוד

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

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

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

את קטע הקוד הבא מצאתי בתשובה של StackOverflow, תשובה שזכתה לכמה upvotes [א].

אז בואו נתחיל.

הקוד

הנה הקוד. במקור הוא נכתב בג׳אווה - אך המרתי אותו לקוטלין (שפה מודרנית יותר), אני מניח שזה לא יקשה על הדיון.

fun balance(amount: Int, round: Boolean, rate: Double, year: Int) : Double {   var yearlyInterestPaid: Double   var totalAmount = amount.toDouble()   for (i in 0..year) { // for (int i = 0; i <= year; i++ ){       yearlyInterestPaid = totalAmount * rate       if (round) yearlyInterestPaid = Math.floor(yearlyInterestPaid * 100) / 100 .       totalAmount += yearlyInterestPaid   }   return totalAmount }

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



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

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

בעיות בולטות במיוחד תופסות את עינינו - ולעתים מסיטות את תשומת הלב שלנו מבעיות חמורות יותר.

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

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

מבט שני

נחזור ונתבונן בקוד, לאחר שהסרנו את הבעיה הבולטת:

fun calcInterest(amount: Int, round: Boolean, rate: Double, year: Int) : Double {   var yearlyInterestPaid: Double   var totalAmount = amount.toDouble()   for (i in 0..year) {       yearlyInterestPaid = totalAmount * rate       if (round) yearlyInterestPaid = Math.floor(yearlyInterestPaid * 100) / 100 .       totalAmount += yearlyInterestPaid   }   return totalAmount}

האם אתם מזהים עכשיו בעיות נוספות?


----


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

הנה הקוד לאחר שני שיפורים נוספים:

fun calcInterest(amount: Double,                 round: Boolean,                 rate: Double,                 year: Int) : Double {     var totalAmount = amount     for (i in 0..year) {       var yearlyInterestPaid = totalAmount * rate       if (round) yearlyInterestPaid = Math.floor(yearlyInterestPaid * 100) / 100 .       totalAmount += yearlyInterestPaid   }     return totalAmount }

אם לא שמתם לב, אז הנה התיקונים (הקטנים) שביצענו:

• הכנסנו את yearlyInterestPaid ל scope מצומצם יותר. אני אזכיר את הכלל הידוע: ״הגדירו משתנים מאוחר ככל האפשר / קרוב ביותר למקום שמשתמשים בהם״.
• מדוע אנו מבצעים המרת טיפוס ל totalAmount? יכול להיות שאין במערכת צורך מיידי בכך, אך הרבה יותר הגיוני שפעולה מתמטית תתבצע על אותו הטיפוס: אם ערך ההחזרה של ה amount לאחר הריבית הוא Double - אז גם שהקלט של ערך הבסיס יהיה מהטיפוס הזה.

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

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

ובכן, כיצד נראית קריאה לפונקציה שלנו?

calcInterest(1000.0, true, 0.04, 10);

מה ניתן להבין מהשורה הזו?

קצת קשה להבין בדיוק מה קורה כאן. מה true? מה בדיוק אומר כל מספר?
האם ייתכן והערך true נוסך בנו יותר בטחון בשימוש בפונקציה מהערך false?! :-)

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

יש IDEs שיוספו כ hint את שמות המשתנים ליד כל ערך - אבל שימו לב ששמות המשתנים לא פותרים את הבעיה:

calcInterest(amount: 1000.0, round: true, rate: 0.04, year: 10);

אם בכדי להבין מה שורה עושה, עלינו להיכנס לקוד הפונקציה - זה לא מצב טוב. בזבזנו זמן, וזו אינדיקציה חזקה מאוד שה "UX של הפונקציה״ (אפשר לקרוא לזה DX = Developer eXperience) - אינה טובה.

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

שיפור ה DX של הפונקציה שלנו

בואו נהפוך את חתימת הפונקציה שלנו לקריאה יותר, וכך - למובנת יותר.

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

כלל אצבע מוכר, הוא לא לשלוח יותר מ 4 פרמטרים לפונקציה. לעתים אנשים חושבים שהבעיה היא חתימה ארוכה של הפונקציה - ואז מעבירים את 12 הפרמטרים ל Request Object אחר בן 12 פרמטרים.

בעצם - מה ההיגיון בזה?!

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

val totalSalary = managersSalary + employeeSalary

עכשיו הופך ל:

val totalSalary = request.managersSalary + request.employeeSalary

זהו עומס נוסף על הקוד, שלא הופך אותו קל יותר לקריאה.

במקום ליצור Request Object אחד, עדיף לייצר 3-4 Request Objects קטנים המקבצים פרמטרים דומים יחדיו.

כך הגישה ל request object היא לא מעמיסה - אלה גם נותנת משמעות לפרמטרים. למשל:

val totalSalary = salaries.managers + salaries.employee

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

כלומר: ב Request Objects טובים יעשה שימוש חוזר - שיפחית את העומס על הקוד.

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

סדר הגיוני של פרמטרים: 

• קיבוץ פרמטרים דומים בזה אחר זה.
• הצבת הפרמטרים החשובים בתחילת הרשימה - והחשובים פחות בסופה.
• אנו רוצים להתחיל לקרוא את חתימת הפונקציה - מהעיקר אל הטפל.

למי שעובד ב IntelliJ, הקיצור CMD+F6 (במק) - הוא קיצור חשוב מאוד. כשאתם מזהים סדר לא נהיר של פרמטרים - הקדישו חצי-דקה וסדרו אותם. IDE מודרני באמת מאפשר לבצע שינוי כזה במהירות ובבטיחות.

הנה הקוד שלנו לאחר השיפור:

fun calcInterest(initialAmount: Double,                 interestRate: Double,                 years: Int,                 roundInterestYearly: Boolean): Double {   var totalAmount = initialAmount   for (i in 0..years) {       var yearlyInterestPaid = totalAmount * interestRate       if (roundInterestYearly) yearlyInterestPaid = floor(yearlyInterestPaid * 100) / 100       totalAmount += yearlyInterestPaid   }   return totalAmount }

שינוי פעוט בשם פרמטר, מ year ל years - הוא משמעותי.

round הוא שם נורא בעיני. הפכנו אותו לשם בעל משמעות.

ההעברה של round לסוף רשימת הפרמטרים מבליטה שהוא:

• פחות חשוב מהפרמטרים האחרים (החישוב העיקרי נעשה בלעדיו)
• קצת פחות קשור לפרמטרים האחרים (שהם הקלט המספרי לנוסחה)

לא סיימנו!

הקוד, אם קיבלנו אותו ל Review לא כולל חלק חיוני - בדיקות יחידה.

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

שאלות מהותיות הן:

• האם אתם מאשרים PR כאשר פונקציה עם Pure Business Logic מוצגת - ללא בדיקת יחידה?
• האם אתם מאשרים PR אם קוד לוגי הוא קרוב להיות טהור, ובמעט עבודה אפשר לזקק אותו - ולבדוק אותו בבדיקות יחידה?

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

מתוך ניסיון ארוך, הכלל גם שימושים שונים ב Floating Point, כשקראתי את התשובה ב StackOverflow - הרגשתי בחוסר נוחות מהדרך שבה ביצעו את ה Rounding. משהו מרגיש פשוט מדי.

לא כתבתי בדיקות יחידה לקוד (אני לרוב כותב, אם אני מפרסם קוד ב SO - כך צריך), אבל מהרצה פשוטה של הקוד ב IDE זיהיתי כבר שני מקרים שעלולים להיות בעייתיים, ובלתי צפויים:

• הפעלת הפונקציה עם סכום של 1000, בריבית הנקובה למעלה, לאורך עשר שנים - מחזירה תוצאה של 1539.4540563150777.
• כאשר אנו מבצעים עיגול כל שנה - הערך הוא עגול (1539.41), אך בהרצה לשש שנים - הערך המתקבל עם הרצת עיגול הוא 1315.9099999999999.
• סכום חיובי וסכום שלילי - לא מתעגלים בצורה סימטרית (כי ה floor הוא אבסולוטי לערך).

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

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


אם אתם מופתעים, ושואלים את עצמכם ״אבל... מדובר ב Stack Overflow - אתר עם ביקורת רבה וקשוחה, איך זה יכול להיות?!״ אני אתן את הציטוט הבא שקראתי בו לאחרונה:

The research echoes an academic paper from 2017 that found 1,161 insecure code snippets posted on Stack Overflow had been copied and pasted into 1.3m Android applications available on Google Play. -- source

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

לא פעם יצא לי למצוא קוד מורכב / מאופטם בצורה חריגה לסביבה שלו - וחיפוש קצר בגוגל הראה שהוא הועתק, as-is, מ StackOverflow. אנחנו עושים את זה, ולא מעט. כולנו במידה כזו או אחרת ״Google Engineers״ (= כאלו שמחפשים בגוגל ואז מעתיקים את התשובה) - אבל חשוב לבדוק את הקוד ולא לסמוך עליו בעיניים עצומות.

סיכום

אשמח לדעת מה דעתכם.

אני מודע לכך שיש גישות שונות, וטעם אישי - ולכן אני מקווה שהדיון יהיה על דברים עקרוניים.

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



----

[א] ליתר דיוק: שילבתי שתי תשובות שונות שזכו ל upvotes - על מנת לחדד כמה נקודות לדיון.