2019-11-30

ללמוד מהקוד

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

כדוגמת קיצון, הנושא של ״למידת מכונה״ יכול לשאוב זמן בלתי-מוגבל - ולא ממש לעזור להיות למתכנת טוב יותר. נושאים כמו 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 - על מנת לחדד כמה נקודות לדיון.



25 תגובות:

  1. אנונימי1/12/19 07:52

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

    השבמחק
  2. אנונימי1/12/19 08:31

    אחלה פוסט!

    השבמחק
  3. אנונימי1/12/19 10:07

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

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

    השבמחק
    תשובות
    1. תודה רבה על הפידבק!

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

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

    השבמחק
    תשובות
    1. תודה רבה על הפידבק!

      ברור ששווה לכתוב בדיקות יחידה (על קוד לוגי)!

      מחק
  5. מחזק את חברי לתגובות - יופי של פוסט!

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

    הנה התוצאה (בטוח אפשר לשפר)
    https://gist.github.com/tzach/e30c92eebc625ae91e3f9c6dba01373c

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

      תודה על הדוגמה!!
      ללמוד Clojure נמצא ב TODO List שלי - אך עדיין לא הגעתי לשם.

      מחק
  7. קודם כל - פוסט מעולה! הערותיי:
    - הייתי מפרט ברשימת בדיקות יחידה (או לפחות את שמות המתודות בצורת given-when-then אם אפשר...)
    - תמיד אפשר לדבר גם על למה הפונקציה מקבלת *כל כך הרבה* פרמטרים...
    - שדה בולאני הוא תמיד רמז לכך שהפונקציה עושה 2 דברים - אולי כדאי לפצל ל2 פונקציות נפרדות? (שוב - מבחינת DX - מה זה אומר הfalse? זה רק אומר שיש IF בפנים שלא חילחל לשם המתודה לדעתי).

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

    השבמחק
    תשובות
    1. תודה על התגובה,

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

      מחק
  8. פוסט מעולה ומחכים, תודה!

    השבמחק
  9. אנונימי8/12/19 10:01

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

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

    השבמחק
    תשובות
    1. שמות משמעותיים (גם אם ארוכים) הייתה גם פרקטיקה שעודדו בשפת C לפני כ 20 שנה. אני זוכר ספר שאמר: מותר לכל משתנה להיות עד 36 תוים - נצלו את האורך עד הסוף. היום כבר אין מגבלות כאלו - אבל מתכנתים עדיין "מתקמצנים" בשמות. Clean Code הוא קונספט דיי אוניברסלי :-).

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

      תודה!

      מחק
  11. אנונימי15/12/19 14:27

    פוסט טוב ומלמד. הייתי שמח לראות יותר כאלה :)
    (אבל לא על חשבון הנושאים המעניינים האחרים שאתה מביא..

    השבמחק