Question from Tapuz .Net forum: Refactoring code for code review

שאלה:

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

מה דעתכם? האם יש דרך לשפר?

        public static void Main()
        {
            //רשימה של השמות הישנים והשמות החדשים על מנת להחליף את הטקסט
            List<manageFiles> mf = new List<manageFiles>();
            mf.Add(new manageFiles("replace me", "with me"));
 
            //רשימת הקבצים החדשה
            List<string> filesUrl = new List<string>();
            filesUrl.Add("myFile1.txt");
            filesUrl.Add("myFile2.txt");
            filesUrl.Add("myFile3.txt");
 
            //מילוי הנתונים
            foreach (string strFile in filesUrl)
            {
                string tochen = "";
                foreach (manageFiles manf in mf)
                {
                    string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
                    Regex rg = new Regex(manf.OldName);
                    using (StreamReader sr = new StreamReader(path))
                    {
                        tochen = sr.ReadToEnd();
                    }
                    //החלפת הערכים
                    tochen = rg.Replace(tochen, manf.NewName);
                    //מחיקת הקובץ הישן
                    if (File.Exists(path))
                    {
                        File.Delete(path);
                    }
                    using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
                    {
                        sw.Write(tochen);
                    }
                }
            }
        }

ובצד יש את המחלקה:

        internal class manageFiles
        {
            public manageFiles(string newName, string oldName)
            {
                this.newName = newName;
                this.oldName = oldName;
            }
 
            private string newName;
            private string oldName;
 
            public string NewName
            {
                get
                {
                    return newName;
                }
            }
 
            public string OldName
            {
                get
                {
                    return oldName;
                }
            }
        }

תשובה:

יש מקום לשיפור ונעבור על זה ביחד דרך כללי Refactoring.

קודם נגדיר מילונית (או אבטיחית) מה זה החיה המוזרה הזאת Refactoring.

Refactoring זה שינוי הקוד הקיים, כך שהוא מבצע בדיוק מה שהוא ביצע קודם, רק כתוב בצורה שונה.

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

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

בואו נתחיל.

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

Main זה לא שם של פונקציה. DoIt זה לא שם של פונקציה. Start זה לא שם של פונקציה.

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

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

נסמן את כל הקוד בתוך Main, נלחץ כפתור ימני, נכנס ל-Refactor ונבחר Extract Method.

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

ועכשיו הקוד שלנו נראה ככה:

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

כלל שני שנראה היום ב-Refactoring - אל תכתבו הערות.

הערות בקוד זה אינהרנטית רע ומסמנות תחלואה עמוקה יותר בקוד.

אני אשאל אתכם את השאלה הקבועה:

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

מתי פעם אחרונה שיניתם הערה שכתבתם כי היא התיישנה? אף פעם.

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

במקום לכתוב בהערות מה שצריך להיות כתוב בקוד, נכתוב מה שהתכוונו בקוד עצמו.

מסתבר שיש מחלקה בשם managedFiles והמטרה שלה היא להחזיק טקסט שעומדים להחליף והטקסט שיחליף אותו.

למה פשוט לא לקרוא למחלקה ככה?

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

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

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

ובמקום הקוד הזה:

            //רשימה של השמות הישנים והשמות החדשים על מנת להחליף את הטקסט
            List<manageFiles> mf = new List<manageFiles>();
            mf.Add(new manageFiles("replace me", "with me"));

נקבל אוטומטית את הקוד הבא:

            //רשימה של השמות הישנים והשמות החדשים על מנת להחליף את הטקסט
            List<PairOfTextToReplaceWithTextToReplaceIt> mf = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            mf.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));

נתנו שם משמעותי למחלקה שאנחנו עובדים איתה.

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

אז בואו נחשוב ביחד, זה אוסף של טקסטים להחלפה והטקסטים שמחליפים אותם...נקרא למשתנה TextsToReplaceWithTextToReplaceThem.

נמחק את השם mf ונכתוב במקומו TextsToReplaceWithTextToReplaceThem.

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

נשנה את השם באופן גורף באמצעות כלי ה-Refactoring של VS2005 והקוד הזה:

            //רשימה של השמות הישנים והשמות החדשים על מנת להחליף את הטקסט
            List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            mf.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));
 
            //רשימת הקבצים החדשה
            List<string> filesUrl = new List<string>();
            filesUrl.Add("myFile1.txt");
            filesUrl.Add("myFile2.txt");
            filesUrl.Add("myFile3.txt");
 
            //מילוי הנתונים
            foreach (string strFile in filesUrl)
            {
                string tochen = "";
                foreach (PairOfTextToReplaceWithTextToReplaceIt manf in mf)

יהפוך לקוד הבא אחריו:

            //רשימה של השמות הישנים והשמות החדשים על מנת להחליף את הטקסט
            List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            TextsToReplaceWithTextToReplaceThem.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));
 
            //רשימת הקבצים החדשה
            List<string> filesUrl = new List<string>();
            filesUrl.Add("myFile1.txt");
            filesUrl.Add("myFile2.txt");
            filesUrl.Add("myFile3.txt");
 
            //מילוי הנתונים
            foreach (string strFile in filesUrl)
            {
                string tochen = "";
                foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)

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

  private static void ReplaceFilesContent()
        {
            List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            TextsToReplaceWithTextToReplaceThem.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));
 
            //רשימת הקבצים החדשה
            List<string> filesUrl = new List<string>();
            filesUrl.Add("myFile1.txt");
            filesUrl.Add("myFile2.txt");
            filesUrl.Add("myFile3.txt");
 
            //מילוי הנתונים
            foreach (string strFile in filesUrl)
            {
                string tochen = "";
                foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)
                {
                    string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
                    Regex rg = new Regex(manf.OldName);
                    using (StreamReader sr = new StreamReader(path))
                    {
                        tochen = sr.ReadToEnd();
                    }
                    //החלפת הערכים
                    tochen = rg.Replace(tochen, manf.NewName);
                    //מחיקת הקובץ הישן
                    if (File.Exists(path))
                    {
                        File.Delete(path);
                    }
                    using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
                    {
                        sw.Write(tochen);
                    }
                }
            }
        }

נטפל גם בהערה הנוספת "רשימת הקבצים החדשה". שלמעשה זהו אוסף (גם באנגלית: Collection) של שמות קבצים שבהם נעבור ונחליף טקסטים. אז נקרא לזה בדיוק ככה.

            //רשימת הקבצים החדשה
            List<string> filesUrl = new List<string>();
            filesUrl.Add("myFile1.txt");
            filesUrl.Add("myFile2.txt");
            filesUrl.Add("myFile3.txt");

הופך ל:

            List<string> URLsForFilesToReplaceTheirContent = new List<string>();
            URLsForFilesToReplaceTheirContent.Add("myFile1.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile2.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile3.txt");

נפנה את תשומת ליבנו לקטע הקוד הבא:

            //מילוי הנתונים

            foreach (string strFile in URLsForFilesToReplaceTheirContent)

            {

                string tochen = "";

                foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)

                {

                    string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;

                    Regex rg = new Regex(manf.OldName);

                    using (StreamReader sr = new StreamReader(path))

                    {

                        tochen = sr.ReadToEnd();

                    }

                    //החלפת הערכים

                    tochen = rg.Replace(tochen, manf.NewName);

                    //מחיקת הקובץ הישן

                    if (File.Exists(path))

                    {

                        File.Delete(path);

                    }

                    using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))

                    {

                        sw.Write(tochen);

                    }

                }

            }

אמרנו כבר, יש כאן הערה, ולכן צריך להוציא את הקוד הזה למתודה נפרדת שהשם שלה יהיה הפונקציונליות שלה.
נסמן את הקוד, נלחץ Ctrl + R, Ctrl + M (קיצור מקלדת ל-Refactor --> Extract Method של העכבר) ונוציא את הקוד הזה למתודה נפרדת.

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

ועכשיו נקרא את באנגלית ביחד:

Replace Text In Fiels( Texts To Replace with Text To Replace Them, URLs For Fiels To Replace Their Content

למישהו יש ספק קל ביותר מה המתודה הזו עושה או מה היא היא מקבלת כפרמטרים?

ככה נראה הקוד שלנו כרגע:

 
        private static void ReplaceFilesContent()
        {
            List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            TextsToReplaceWithTextToReplaceThem.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));
 
            List<string> URLsForFilesToReplaceTheirContent = new List<string>();
            URLsForFilesToReplaceTheirContent.Add("myFile1.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile2.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile3.txt");
 
            ReplaceTextInFiels(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }
 
        private static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            //מילוי הנתונים
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string tochen = "";
                foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)
                {
                    string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
                    Regex rg = new Regex(manf.OldName);
                    using (StreamReader sr = new StreamReader(path))
                    {
                        tochen = sr.ReadToEnd();
                    }
                    //החלפת הערכים
                    tochen = rg.Replace(tochen, manf.NewName);
                    //מחיקת הקובץ הישן
                    if (File.Exists(path))
                    {
                        File.Delete(path);
                    }
                    using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
                    {
                        sw.Write(tochen);
                    }
                }
            }
        }

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

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

        private static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {

            //מילוי הנתונים
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string tochen = "";
                foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)
                {
                       ...
                }
            }
         }

הופך ל:

        private static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            IterateOverAllFilesToReplaceTheirContent(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }
 
        private static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string tochen = ""; foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)
                {
                    string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
                    Regex rg = new Regex(manf.OldName);
                    using (StreamReader sr = new StreamReader(path))
                    {
                        tochen = sr.ReadToEnd();
                    }
                    //החלפת הערכים
                    tochen = rg.Replace(tochen, manf.NewName);
                    //מחיקת הקובץ הישן
                    if (File.Exists(path))
                    {
                        File.Delete(path);
                    }
                    using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
                    {
                        sw.Write(tochen);
                    }
                }
            }
        }

ואז ל:

      private static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> 
TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            IterateOverAllFilesToReplaceTheirContent(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }
 
        private static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string tochen = "";
                tochen = ReplaceAllTextsInCurrentFile(TextsToReplaceWithTextToReplaceThem, strFile, tochen);
            }
        }
 
        private static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string tochen)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)
            {
                   ....
            }
            return tochen;
        }

למעשה כתבנו כאן תיעוד - אמרנו שהלולאה הראשונה היא IterateOverAllFilesToReplaceTheirContent והלולאה השנייה היא ReplaceAllTextsInCurrentFile. הבהרנו למען קריאות בעתיד, למה בכלל כתבנו את הלולאה הזאת.

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

שימו לב אגב מה קיבלנו בפונקציה הראשונה:

        private static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            IterateOverAllFilesToReplaceTheirContent(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }

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

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

נעבור לשורת קוד הזו:

                string tochen = "";

לא כותבים פונטית אף-פעם.

בשפה שבה אתה כותב - זאת השפה שבאוצר מילים שלה אתה תשתמש.

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

        private static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string CurrentContentOfFile = "";
                CurrentContentOfFile = ReplaceAllTextsInCurrentFile(TextsToReplaceWithTextToReplaceThem, strFile, CurrentContentOfFile);
            }
        }

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

        private static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string תוכןהקובץ = "";
                תוכןהקובץ = ReplaceAllTextsInCurrentFile(TextsToReplaceWithTextToReplaceThem, strFile, תוכןהקובץ);
            }
        }

זה חוקי לחלוטין מבחינת הקומפיילר והקוד הזה יעבור קומפילציה.

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

הראשונה, עברית לא נתמכת טוב ב-Visual Studio או בשום Add-in. אורן עיני כתב על זה פוסט נהדר שמראה את המכשולים הטכניים לכתוב בעברית.

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

השלישית, עברית היא שפה נטולת Capital Letters וצריך להסתמך על קו תחתון _ בשבילה.

נחזור לשורה הזו

                string CurrentContentOfFile = "";

עדיף לאתחל פרמטר string עם String.Empty ולא עם גרשיים כפולות "".

                string CurrentContentOfFile = string.Empty;

נחזור למתודה שאנחנו כרגע עובדים עליה:

        private static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string CurrentContentOfFile)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt manf in TextsToReplaceWithTextToReplaceThem)
            {
                string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
                using (StreamReader sr = new StreamReader(path))
                {
                    CurrentContentOfFile = sr.ReadToEnd();
                }
                //החלפת הערכים
                Regex rg = new Regex(manf.OldName);
                CurrentContentOfFile = rg.Replace(CurrentContentOfFile, manf.NewName);
                //מחיקת הקובץ הישן
                if (File.Exists(path))
                {
                    File.Delete(path);
                }
                using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
                {
                    sw.Write(CurrentContentOfFile);
                }
            }
            return CurrentContentOfFile;
        }

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

- השם פרמטר manf לא באמת אומר כלום על מה הפרמטר עושה ומה המטרה שלו. נשנה אותו ל-currentPairToBeUsedForReplacing.

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

        private static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string CurrentContentOfFile)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing in TextsToReplaceWithTextToReplaceThem)
            {
                string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
 
                CurrentContentOfFile = GetContentFromFile(CurrentContentOfFile, path);
 
                CurrentContentOfFile = ReplaceFileContentsForCurrentTextToBeReplaced(CurrentContentOfFile, currentPairToBeUsedForReplacing);
 
                DeleteOldFile(path);
 
                WriteNewFile(CurrentContentOfFile, path);
            }
            return CurrentContentOfFile;
        }
 
        private static void WriteNewFile(string CurrentContentOfFile, string path)
        {
            using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
            {
                sw.Write(CurrentContentOfFile);
            }
        }
 
        private static void DeleteOldFile(string path)
        {
 
            if (File.Exists(path))
            {
                File.Delete(path);
            }
        }
 
        private static string ReplaceFileContentsForCurrentTextToBeReplaced(string CurrentContentOfFile, PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing)
        {
            Regex rg = new Regex(currentPairToBeUsedForReplacing.OldName);
            CurrentContentOfFile = rg.Replace(CurrentContentOfFile, currentPairToBeUsedForReplacing.NewName);
            return CurrentContentOfFile;
        }
 
        private static string GetContentFromFile(string CurrentContentOfFile, string path)
        {
            using (StreamReader sr = new StreamReader(path))
            {
                CurrentContentOfFile = sr.ReadToEnd();
            }
            return CurrentContentOfFile;
        }

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

שימו לב למתודה הראשית:

        private static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string CurrentContentOfFile)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing in TextsToReplaceWithTextToReplaceThem)
            {
                string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;
 
                CurrentContentOfFile = GetContentFromFile(CurrentContentOfFile, path);
 
                CurrentContentOfFile = ReplaceFileContentsForCurrentTextToBeReplaced(CurrentContentOfFile, currentPairToBeUsedForReplacing);
 
                DeleteOldFile(path);
 
                WriteNewFile(CurrentContentOfFile, path);
            }
            return CurrentContentOfFile;
        }

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

עבור כל זוג להחלפה:

1. תשיג תוכן הקובץ

2. תשנה את תוכן הקובץ

3. תמחק את הקובץ הישן

4. תכתוב את הקובץ עם התוכן החדש

זה בדיוק מה שכתוב בקוד שלנו! שום הערות, שום תיעוד מסורבל, הכל מובע וכתוב בקוד שלנו.

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

ראינו כמה קונספטים מעניינים:

1. אחריות של מתודות - כל מתודה מבצעת דבר אחד ודבר אחד בלבד

2. הסתרה של אופן ביצוע - המתודה "הראשית" (ביחס לתהליך שלנו) לא חושפת מה בדיוק היא מבצעת.

נמשיך עם השורה הבאה:

                string path = HttpContext.Current.Server.MapPath("../../files/") + strFile;

נתנו למתודה ReplaceAllTextsInCurrentFile את האחריות לגלות את הנתיב המלא של הקובץ.

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

                string path = GetPathForFile(strFile);
 
         ...
 
        private static string GetPathForFile(string strFile)
        {
            return HttpContext.Current.Server.MapPath("../../files/") + strFile;
        }

עכשיו נראה מתודה את המתודה הבאה:

        private static void WriteNewFile(string CurrentContentOfFile, string path)
        {
            using (StreamWriter sw = new StreamWriter(path, true, System.Text.Encoding.UTF8))
            {
                sw.Write(CurrentContentOfFile);
            }
        }

יש לנו בעיה עם הקוד הזה. בואו נראה את הקונסטרקטור (גם באנגלית: Contructor) של StreamWriter.

כרגע נגדיר שתמיד שנרצה לפתוח StreamWriter באפליקציה שלנו ולא רק במתודה הזו - אנחנו נרצה כברירת מחדל שהוא יהיה Append=true עם קידוד UTF8. שזה מצב מאוד הגיוני לאפליקציה שתומכת בעברית. במקום לאתחל ב-40 מקומות באפליקציה StreamWriter עם הפרמטרים האלו נרצה לרכז איפשהו אותם.

נרצה להגיד שכברירת מחדל באפליקציה שלנו עובדים עם append=true ו-encoding של UTF8.

נרצה גם שהמקום הזה שאומר את זה, יחזיר לנו מופע מאותחל של StreamWriter.

מסתבר שיש משהו שנקרא Factory Design Pattern שהמטרה שלו היא לענות בדיוק על מקרים כאלו.
ניצור מחלקה סטטית בצד שתהיה StreamWriterFactory ותחזיר לנו מופע חדש של StreamWriter.

        private static void WriteNewFile(string CurrentContentOfFile, string path)
        {
            using (StreamWriter sw = SteamWriterFactory.CreateStreamWriter(path))
            {
                sw.Write(CurrentContentOfFile);
            }
        }
 
        public class SteamWriterFactory
        {
            public static StreamWriter CreateStreamWriter(string path)
            {
                return new StreamWriter(path, true, System.Text.Encoding.UTF8);
            }
        }

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

נביט על מתודה נוספת

        private static string ReplaceFileContentsForCurrentTextToBeReplaced(string CurrentContentOfFile, PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing)
        {
            Regex rg = new Regex(currentPairToBeUsedForReplacing.OldName);
            CurrentContentOfFile = rg.Replace(CurrentContentOfFile, currentPairToBeUsedForReplacing.NewName);
            return CurrentContentOfFile;
        }

במתודה הזו, אנחנו משתמשים ב-PairOfTextToReplaceWithTextToReplaceIt כדי להחליט איזה טקסט מוחלף באיזה טקסט.

לי אישית, לא ברור ש-oldName זה הטקסט שמוחלף ו-newName זה הטקסט שמחליף אותו. נלך למחלקה שמכילה אותם ונביט עליה

        internal class PairOfTextToReplaceWithTextToReplaceIt
        {
            public PairOfTextToReplaceWithTextToReplaceIt(string newName, string oldName)
            {
                this.newName = newName;
                this.oldName = oldName;
            }
 
            private string newName;
            private string oldName;
 
            public string NewName
            {
                get
                {
                    return newName;
                }
            }
            public string OldName
            {
                get
                {
                    return oldName;
                }
            }
        }

נוכל באמצעות VS2005 לשנות את שמות המאפיינים (גם באנגלית: Properties) ו-VS ידאג עבורנו לשנות את שמות ה-Propert בכל מקום אחר.

מחוץ למחלקה יש למאפיינים את השמות החדשים.

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

VS2005 לא תומך בזה, בשביל זה יש מוצרים צד שלישי כמו Resharper, כמו CodeSmart, כמו CodeItOnce ורבים נוספים.

נדגים שימוש ב-Resharper.

נשנה גם את NewName ל-TextToBeReplacedWith.

        internal class PairOfTextToReplaceWithTextToReplaceIt
        {
            public PairOfTextToReplaceWithTextToReplaceIt(string textToBeReplacedWith, string textToReplace)
            {
                this.textToBeReplacedWith = textToBeReplacedWith;
                this.textToReplace = textToReplace;
            }
 
            private string textToBeReplacedWith;
            private string textToReplace;
 
            public string TextToBeReplacedWith
            {
                get
                {
                    return textToBeReplacedWith;
                }
            }
            public string TextToReplace
            {
                get
                {
                    return textToReplace;
                }
            }
        }

וכמובן שבאמצעות VS2005 או באמצעות מוצרי צד-שלישי הקוד מחוץ למחלקה יראה אותו דבר:

        private static string ReplaceFileContentsForCurrentTextToBeReplaced(string CurrentContentOfFile, PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing)
        {
            Regex rg = new Regex(currentPairToBeUsedForReplacing.TextToReplace);
            CurrentContentOfFile = rg.Replace(CurrentContentOfFile, currentPairToBeUsedForReplacing.TextToBeReplacedWith);
            return CurrentContentOfFile;
        }

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

        private static string ReplaceFileContentsForCurrentTextToBeReplaced(string CurrentContentOfFile, PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing)
        {
            Regex regexUsedToReplaceText = new Regex(currentPairToBeUsedForReplacing.TextToReplace);
            CurrentContentOfFile = regexUsedToReplaceText.Replace(CurrentContentOfFile, currentPairToBeUsedForReplacing.TextToBeReplacedWith);
            return CurrentContentOfFile;
        }

עכשיו בואו נראה דוגמה למה כל ה-Refactoring הזה טוב...

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

אפשר גם לראות את זה בקוד שלנו:

        private static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string CurrentContentOfFile = string.Empty;
                CurrentContentOfFile = ReplaceAllTextsInCurrentFile(TextsToReplaceWithTextToReplaceThem, strFile, CurrentContentOfFile);
            }
        }
 
        private static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string CurrentContentOfFile)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing in TextsToReplaceWithTextToReplaceThem)
            {
                string path = GetPathForFile(strFile);
 
                CurrentContentOfFile = GetContentFromFile(CurrentContentOfFile, path);
 
                CurrentContentOfFile = ReplaceFileContentsForCurrentTextToBeReplaced(CurrentContentOfFile, currentPairToBeUsedForReplacing);
 
                DeleteOldFile(path);
 
                WriteNewFile(CurrentContentOfFile, path);
            }
            return CurrentContentOfFile;
        }

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

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

        private static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string path = GetPathForFile(strFile);
 
                string CurrentContentOfFile = GetContentFromFile(path);
 
                CurrentContentOfFile = ReplaceAllTextsInCurrentFile(TextsToReplaceWithTextToReplaceThem, strFile, CurrentContentOfFile);
 
                DeleteOldFile(path);
 
                WriteNewFile(CurrentContentOfFile, path);
            }
        }
 
        private static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string CurrentContentOfFile)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing in TextsToReplaceWithTextToReplaceThem)
            {
                CurrentContentOfFile = ReplaceFileContentsForCurrentTextToBeReplaced(CurrentContentOfFile, currentPairToBeUsedForReplacing);
            }
            return CurrentContentOfFile;
        }

באמצעות Refactoring גם ראינו את הבעיה בצורה הרבה יותר ברורה (כי בפעם הראשונה נחשף לענינו האלגוריתם שמימשנו) וגם היה לנו הרבה יותר קל לבצע שינויים.

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

        public static void Main()
        {
            ReplaceFilesContent();
        }
 
        private static void ReplaceFilesContent()
        {
            List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            TextsToReplaceWithTextToReplaceThem.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));
 
            List<string> URLsForFilesToReplaceTheirContent = new List<string>();
            URLsForFilesToReplaceTheirContent.Add("myFile1.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile2.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile3.txt");
 
            ReplaceTextInFiels(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }
 
        private static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            IterateOverAllFilesToReplaceTheirContent(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }

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

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

    public static class contentReplacer
    {
 
    }

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

VS2005 לא תומך בזה, ולכן אם נאלץ לעבוד איתו נצטרך להעביר ידנית מתודה-מתודה.

כלים צד-שלישי שציינתי קודם כן תומכים בתהליך. למשל באמצעות Resharper.

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

ובסופו של דבר הקוד שלנו יראה ככה:

    public class RefactoringExample
    {
        public static void Main()
        {
            ReplaceFilesContent();
        }
 
        private static void ReplaceFilesContent()
        {
            List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem = new List<PairOfTextToReplaceWithTextToReplaceIt>();
            TextsToReplaceWithTextToReplaceThem.Add(new PairOfTextToReplaceWithTextToReplaceIt("replace me", "with me"));
 
            List<string> URLsForFilesToReplaceTheirContent = new List<string>();
            URLsForFilesToReplaceTheirContent.Add("myFile1.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile2.txt");
            URLsForFilesToReplaceTheirContent.Add("myFile3.txt");
 
            ContentReplacer.ReplaceTextInFiels(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }
    }
 
    public class PairOfTextToReplaceWithTextToReplaceIt
    {
        public PairOfTextToReplaceWithTextToReplaceIt(string textToBeReplacedWith, string textToReplace)
        {
            this.textToBeReplacedWith = textToBeReplacedWith;
            this.textToReplace = textToReplace;
        }
 
        private string textToBeReplacedWith;
        private string textToReplace;
 
        public string TextToBeReplacedWith
        {
            get
            {
                return textToBeReplacedWith;
            }
        }
        public string TextToReplace
        {
            get
            {
                return textToReplace;
            }
        }
    }
 
    public class SteamWriterFactory
    {
        public static StreamWriter CreateStreamWriter(string path)
        {
            return new StreamWriter(path, true, System.Text.Encoding.UTF8);
        }
    }
 
    public static class ContentReplacer
    {
        public static void ReplaceTextInFiels(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            IterateOverAllFilesToReplaceTheirContent(TextsToReplaceWithTextToReplaceThem, URLsForFilesToReplaceTheirContent);
        }
 
        public static void IterateOverAllFilesToReplaceTheirContent(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, List<string> URLsForFilesToReplaceTheirContent)
        {
            foreach (string strFile in URLsForFilesToReplaceTheirContent)
            {
                string path = GetPathForFile(strFile);
 
                string CurrentContentOfFile = GetContentFromFile(path);
 
                CurrentContentOfFile = ReplaceAllTextsInCurrentFile(TextsToReplaceWithTextToReplaceThem, strFile, CurrentContentOfFile);
 
                DeleteOldFile(path);
 
                WriteNewFile(CurrentContentOfFile, path);
            }
        }
 
        public static string ReplaceAllTextsInCurrentFile(List<PairOfTextToReplaceWithTextToReplaceIt> TextsToReplaceWithTextToReplaceThem, string strFile, string CurrentContentOfFile)
        {
            foreach (PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing in TextsToReplaceWithTextToReplaceThem)
            {
                CurrentContentOfFile = ReplaceFileContentsForCurrentTextToBeReplaced(CurrentContentOfFile, currentPairToBeUsedForReplacing);
            }
            return CurrentContentOfFile;
        }
 
        public static string GetPathForFile(string strFile)
        {
            return HttpContext.Current.Server.MapPath("../../files/") + strFile;
        }
 
        public static void WriteNewFile(string CurrentContentOfFile, string path)
        {
            using (StreamWriter sw = SteamWriterFactory.CreateStreamWriter(path))
            {
                sw.Write(CurrentContentOfFile);
            }
        }
 
        public static void DeleteOldFile(string path)
        {
            if (File.Exists(path))
            {
                File.Delete(path);
            }
        }
 
        public static string ReplaceFileContentsForCurrentTextToBeReplaced(string CurrentContentOfFile, PairOfTextToReplaceWithTextToReplaceIt currentPairToBeUsedForReplacing)
        {
            Regex regexUsedToReplaceText = new Regex(currentPairToBeUsedForReplacing.TextToReplace);
            CurrentContentOfFile = regexUsedToReplaceText.Replace(CurrentContentOfFile, currentPairToBeUsedForReplacing.TextToBeReplacedWith);
            return CurrentContentOfFile;
        }
 
        public static string GetContentFromFile(string path)
        {
            string CurrentContentOfFile;
            using (StreamReader sr = new StreamReader(path))
            {
                CurrentContentOfFile = sr.ReadToEnd();
            }
            return CurrentContentOfFile;
        }
    }

הקוד שקיבלנו הוא:

1. הרבה יותר קריא

2. הרבה יותר ברור מה המטרה של הקוד כ-כלל

3. הרבה יותר ברור מה המטרה של כל שורת קוד פונקציונלית שכתבנו

4. יותר קל לשנות אותו ולעשות שינויים של האלגוריתם וגם באופן הביצוע הפנימי שלו

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

נעבור בקצרה על מה ראינו:

1. נתינת שמות משמעותיים למחלקות שמכילות מידע. איך לשנות את השם שלהן באמצעות VS2005.

2. נתינת שמות משמעותיים ל-Properties של מחלקות. איך לשנות את השם של Propert באמצעות VS2005, ואיך מוצרים צד-שלישי מאפשרים שינוי מקיף של כל המחלקה ולא רק שם ה-Property.

3. ברוב המוחץ של המקרים נעדיף להימנע מלכתוב הערות ונעדיף קוד שמתעד את עצמו.

4. נתינת שמות משמעותיים למשתנים פנימיים.

5. חלוקת אחריות בין מחלקות ומתודות שונות.

6. ההטבות ש-Refoactoring מביא לנו מבחינת קריאות ויכולת תחזוקת ושינוי הקוד.

7. פתרון בעיות נפוצות באמצעות Design Patterns.

8. אין שכפול קוד ברחבי המערכת.

לקוד הזה יש עוד כברת דרך לעבור. גם מבחינת Refactoring וגם היות שהוא עדיין לא Testabily. עדיין אי-אפשר לכתוב קוד שבודק את הקוד הזה באמצעות Unit Testing היות והוא ניגש ישירות ל-HD מה שהופך אותו ל-Intergration Testing. אז נדרש שם עוד שינוי ארכיטקטורי בשביל זה. אבל טסטביליות זה נושא נפרד מ-Refactoring וזה ליום אחר.

קישור: http://www.tapuz.co.il/tapuzforum/main/Viewmsg.asp?forum=831&msgid=100306278

Published Wednesday, June 13, 2007 2:11 PM by Justin-Josef Angel [MVP]

תגים:Tapuz, OOP, Refactoring, Design Patterns

Comments

No Comments

Justin myJustin = new Justin( Expriences.Current );

This Blog

Syndication

Search

Tags

News

Go to my Article Index

Go to my Article Index

Navigation

Archives

Blogs I read

Question from Tapuz .Net forum: Refactoring code for code review

Comments