ככה מצאתי את זה

בשיטוט מקרי בקוד של axios נתקלתי בקוד הבא עבור אחת הבדיקות:

// from: https://github.com/axios/axios/blob/v1.x/test/specs/promise.spec.js#L10

it('should provide succinct object to then', function (done) {
  let response;

  axios('/foo').then(function (r) {
    response = r;
  });

  getAjaxRequest().then(function (request) {
    request.respondWith({
      status: 200,
      responseText: '{"hello":"world"}'
    });

    setTimeout(function () {
      expect(typeof response).toEqual('object');
      expect(response.data.hello).toEqual('world');
      expect(response.status).toEqual(200);
      expect(response.headers['content-type']).toEqual('application/json');
      expect(response.config.url).toEqual('/foo');
      done();
    }, 100);
  });
});

הם מגדירים שרת פיקטיבי ומאתחלים את פרטי התשובה הפיקטיבית שהוא ישלח, שולחים בקשת רשת עם axios ואז מחכים 100 מילי שניות ובודקים את אוביקט התשובה. אבל למה לחכות דווקא 100 מילי שניות? לפי גיט בליים האשמה היא בקומיט 46a9639. כל הבדיקות עד אותו קומיט השתמשו ב setTimeout עם זמן המתנה של 0, אבל חלקן כנראה נכשלו מדי פעם ובאותו קומיט כל הבדיקות שכללו setTimeout עלו לזמן המתנה 100 מילי שניות.

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

  it('should provide succinct object to then', function (done) {
    axios('/foo').then(function (response) {
      expect(typeof response).toEqual('object');
      expect(response.data.hello).toEqual('world');
      expect(response.status).toEqual(200);
      expect(response.headers['content-type']).toEqual('application/json');
      expect(response.config.url).toEqual('/foo');
      done();
    });

    getAjaxRequest().then(function (request) {
      request.respondWith({
        status: 200,
        responseText: '{"hello":"world"}'
      });
    });
  });

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

אבל יש שתי בעיות בגישה כזאת:

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

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

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