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

1. ספירת מילים בקובץ בפרל, בדרך הלא נכונה

הקוד הבא הופיע בבלוג אחר והכותב הציג אותו כגירסת פרל לתוכנית הסופרת מילים בקובץ:

#!/usr/bin/env perl

use strict;
use warnings;

run(\@ARGV);

sub run {
    my $argv = shift;
    my @counts;

    for my $file ( @$argv ) {
        my $count = -1;
        eval {
            $count = word_count($file);
            1;
        } or warn "$@";

        push @counts, {
            file => $file,
            word_count => $count,
        };
    }

    for my $result (@counts) {
        printf "%s: %d words\n", $result->{file}, $result->{word_count};
    }
}

sub word_count {
    my $file = shift;
    my %words;

    open my $fh, '<', $file
        or die "Cannot open '$file': $!";

    while (my $line = <$fh>) {
        my @words = split ' ', $line;
        $words{ $_ } += 1 for @words;
    }

    close $fh;

    my $word_count;
    $word_count += $_ for values %words;
    return $word_count;
}

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

2. הקוד עושה הרבה עבודה מיותרת

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

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

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

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

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

 

3. הקוד לא משתמש ביכולות של השפה

אני נשאר באותה הפונקציה word_count. בסופה הכותב סוכם את כל הערכים מתוך המשתנה %words. זו הלולאה:

my $word_count;
$word_count += $_ for values %words;
return $word_count;

אבל רגע — בפרל יש פונקציה שנקראת sum. נכון שבשביל להגיע אליה צריך להוסיף את השורה use List::Util 'sum' בראש התוכנית אבל מדובר במודול ליבה שמגיע עם כל הפצה של פרל, כלומר זה חלק מהשפה. כך זה היה יכול להיראות במקום כל שלושת השורות (אנחנו בסוף הפונקציה לכן המילה return מיותרת כאן):

sum(values %words)

וזה לא הכל. שימו לב לשורות:

while (my $line = <$fh>) {
    my @words = split ' ', $line;
    $words{ $_ } += 1 for @words;
}

הקוד מפריד את השורה למילים וסופר מופעים של כל מילה ב Hash. אבל הרי פרל מאפשרת כתיב הרבה יותר נקי של הרעיון בלי שימוש במשתנה העזר $line באופן הבא:

while (<$fh>) {
    my @words = split;
    $words{$_} += 1 for @words;
}

 

4. הקוד לא משתמש בספריות מקובלות

הספריה החיצונית Try::Tiny היתה יכולה להפוך את בלוק ה Eval לקריא יותר עבור משתמשים שאינם בקיאים בפרל. כך זה היה יכול להיראות:

my $count = -1;
try {
    $count = word_count($file);
}
catch {
    warn "$_";
};

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

5. ואיך היה אפשר לכתוב את זה פשוט יותר?

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

system("wc", @ARGV);

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

#!/usr/bin/env perl

use strict;
use warnings;

my %counts = map { $_ => 0 } @ARGV;
while(<>) {
    $counts{$ARGV} += split;
}

my @counts = map {{
        file       => $_,
        word_count => $counts{$_}
    }} keys %counts;

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