setInterval

[shpro654]

דף שמוצג על מסך פלאזמה, מה שאומר שעקרונית, יתכן ופותחים אותו ולא סוגרים אותו במשך חודשים.
מה שעשיתי כרגע:
setInterval - כל 15 דקות, קריאה לשרת.
ו- http-equiv=refresh (ב-html) אחת ליום / כמה שעות.
האם זו הדרך הנכונה?

---

הפונקציה שקוראת לשרת (כזכור ב- setInterval כל 15 דק'),
קוראת לפונקצית המשך שבתוכה יש setInterval פנימי.
ה-clearInterval הפנימי קורא שוב לפונקציה שבה הוא נמצא (רקורסיה(?)
עשיתי clearInterval ל-set הפנימי, בכל קריאה חדשה של setInterval לפונקציה החיצונית,
(וזו הסיבה למה השתמשתי ב-setInterval ולא ב-setTimeOut לרקורסיה)
אך לא רק שזה לא מתאפס, אלא הרקורסיה משתכפלת בריבוע.

[יוסף בן שמעון]

@shpro654 אמר בsetInterval:

האם זו הדרך הנכונה?

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

פעם נדרשתי למשהו דומה, עשיתי בדף קבוע שמאותחל עם טעינת הדף, ושומר את הזמן הנוכחי של טעינת הדף, ובשרת עשיתי משתנה שבכל עדכון או אתחול של השרת שומר את הזמן הנוכחי, הדף שולח מידי פעם בקשה לשרת ומקבל את הזמן עדכון האחרון, ואם זמן העדכון מאוחר מזמן הטעינה הדף מרפרש את עצמו
כדי לרפרש דף אפשר לשלוח הוראה מהשרת כמו שעשית, ואפשר לעשות את זה מקומי עם window.location.reload()

@shpro654 אמר בsetInterval:

הפונקציה שקוראת לשרת (כזכור ב- setInterval כל 15 דק'),
קוראת לפונקצית המשך שבתוכה יש setInterval פנימי.
ה-clearInterval הפנימי קורא שוב לפונקציה שבה הוא נמצא (רקורסיה(?)
עשיתי clearInterval ל-set הפנימי, בכל קריאה חדשה של setInterval לפונקציה החיצונית,
(וזו הסיבה למה השתמשתי ב-setInterval ולא ב-setTimeOut לרקורסיה)
אך לא רק שזה לא מתאפס, אלא הרקורסיה משתכפלת בריבוע.

אולי תביא קוד להבהיר במה מדובר

[dovid]

@shpro654 אמר בsetInterval:

הפונקציה שקוראת לשרת (כזכור ב- setInterval כל 15 דק'),
קוראת לפונקצית המשך שבתוכה יש setInterval פנימי.
ה-clearInterval הפנימי קורא שוב לפונקציה שבה הוא נמצא (רקורסיה(?)
עשיתי clearInterval ל-set הפנימי, בכל קריאה חדשה של setInterval לפונקציה החיצונית,
(וזו הסיבה למה השתמשתי ב-setInterval ולא ב-setTimeOut לרקורסיה)
אך לא רק שזה לא מתאפס, אלא הרקורסיה משתכפלת בריבוע.

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

[shpro654]

תיאור:
המטרה היא להציג מודעות (jpg), שמגיעות בקריאה ב-api של גוגל דרייב (כן, נכון, ה-cros וכו').
לפונקציה הראשית יש setInterval כל 15 דקות לקריאה לשרת. בודק אם השתנו / נוספו מודעות.

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

1. ה-Interval הפנימי משתכפל בחזקה.
2. ה-clearInterval לא מאפס את את ה-Interval הפנימי.
3. האם זו הדרך הנכונה בכלל לפתור את הבעיה?
4. code review... אשמח מאוד. תודה.

ולקוד בהרחבה:

let aSliderInterval = ''

const getAds = () => {

    fetch... {

        const { files: ads, LastUpdated } = data;

            let adsLength = ads.length;
            if (adsLength > 0) {
                for (let i = 0; i < adsLength; i++) {
                    const { fileDateCreated, name } = ads[i];
                    if (fileDateCreated > newUpdate) {
                        newUpdate = fileDateCreated;
                    }
                    if (adsLength < adminNote.adsLength) {
                        adminNote.adsLength = adsLength;
                        clearInterval(aSliderInterval);
                        setAds(ads, newUpdate);
                        return
                    }
                    if (newUpdate <= adminNote.update) {
                        return
                    }
                }
                adminNote.adsLength = adsLength;
                clearInterval(aSliderInterval);
                setAds(ads, newUpdate);
            }
        }
}


function setAds(ads, newUpdate) {

    const adsSlider = document.querySelector('#ads-slider');
    adsSlider.innerHTML = '';
    const adsFooter = document.querySelector('.a-footer');
    let adsLength = ads.length;

    for (let i = 0; i < adsLength; i++) {
        const adImg = document.createElement('img');
        adImg.className = 'ad ad-none';
        adImg.src = `https://drive.google.com/uc?id=${ads[i].id}`;
        adsSlider.appendChild(adImg);
    }

    adminNote.update = newUpdate;

    aSlider(0, adsLength > 3 ? 4 : adsLength, adsLength);
}

function aSlider(from, to, adsLength) {

    console.log('aSlider');
    const adsDom = document.querySelectorAll('.ad');

    for (let i = 0; i < adsLength; i++) {
        adsDom[i].className = 'ad ad-none';
    }

    for (let i = from; i < to; i++) {
        adsDom[i].className = 'ad';
    }

    const adsFooter = document.querySelector('.a-footer');
    adsFooter.innerHTML = `מודעות ${from + 1}-${to} מתוך ${adsLength}`;


    if (adsLength > 4) {
        aSliderInterval = setInterval(() => {
            if (adsLength <= to) to = 0;
            aSlider(to, to + 4 > adsLength ? adsLength : to + 4, adsLength)
        }, 30000);
    }
}

getAds();

setInterval(() => {
    getAds();
}, 900000); //15 min

[dovid]

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

[shpro654]

@dovid אמר בsetInterval:

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

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

@dovid אמר בsetInterval:

כשאתה רוצה ערך נמוך או גבוה מבין שניים הרבה יותר קריא להשתמש בMath.max וmin בהתאמה.

לגבי מה? (איפה בקוד?)

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

רענון הדף אחת ליום או משהו כזה, "ליתר בטחון", כן?

תודה רבה!

[dovid]

 adsLength > 3 ? 4 : adsLength

תשנה ל

Math.min(adsLength, 4)

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

@shpro654 אמר בsetInterval:

רענון הדף אחת ליום או משהו כזה, "ליתר בטחון", כן?

כן.

[shpro654]

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

let aSliderInterval = ''

const getAds = () => {

    fetch... {
            const { files: ads } = data;
            checkIfFolderChanghd(ads) && setAds(ads);
    }
}

function checkIfFolderChanghd(ads) {

    const adminAdsLength = adminSettings.adsLength
    let adsLength = ads.length;
    let newUpdate = '';

    if (!adsLength) return true;

    if (adsLength < adminAdsLength || adsLength > adminAdsLength) {
        return true;
    }

    for (let i = 0; i < adsLength; i++) {
        const { fileDateCreated } = ads[i];
        if (fileDateCreated > newUpdate) {
            newUpdate = fileDateCreated;
        }
    }

    if (newUpdate <= adminSettings.update) {
        return false;
    }

    return true;
}


function setAds(ads) {

    let newUpdate = '';
    let adsLength = ads.length;

    const adsSlider = document.querySelector('#ads-slider');
    adsSlider.innerHTML = '';

    if (!adsLength) {
        aSliderInterval && clearInterval(aSliderInterval);
        setBoardFooter(0, 0, 0);
        return
    }

    for (let i = 0; i < adsLength; i++) {
        const { fileDateCreated, id } = ads[i];

        const adImg = document.createElement('img');
        adImg.className = 'ad ad-none';
        adImg.src = `https://drive.google.com/uc?id=${id}`;
        adsSlider.appendChild(adImg);

        if (fileDateCreated > newUpdate) {
            newUpdate = fileDateCreated;
        }
    }

    adminSettings.update = newUpdate;
    adminSettings.adsLength = adsLength;

    const { maxAds } = adminSettings;
    aSliderInterval && clearInterval(aSliderInterval);
    aSlider(0, Math.min(maxAds, adsLength), adsLength);

    if (adsLength > maxAds) {

        aSliderInterval = setInterval(() => {
            const { from, to } = adminSettings;
            aSlider(from, to, adsLength);
        }, 30000); //30 s
    }
}


function aSlider(from, to, adsLength) {

    const { maxAds } = adminSettings;
    const adsDom = document.querySelectorAll('.ad');

    for (var ad of adsDom) {
        ad.className = 'ad ad-none';
    }

    for (let i = from; i < to; i++) {
        adsDom[i].className = 'ad';
    }

    setBoardFooter(from, to, adsLength);

    if (to === adsLength) {
        adminSettings.from = 0;
        adminSettings.to = maxAds;
    }
    else {
        adminSettings.from = to;
        adminSettings.to = Math.min(adsLength, to + maxAds);
    }
}


function setBoardFooter(from, to, lebgth) {
    const adsFooter = document.querySelector('.a-footer');

    if (!lebgth) return adsFooter.innerHTML = 'אין מודעות';

    adsFooter.innerHTML = `מודעות ${from + 1}-${to} מתוך ${lebgth}`;
}

setInterval(() => {
    getAds();
}, 900000); //15 min

ושאלה על הדרך (ת"מ):
מה הדרך הנכונה / המקובלת לשרשור פונקציות (מ-get, ל-set, ל-slider)
בשרשור מתוך הפונקציות כמו שעשיתי,
או במרכז אחד שמטפל, מקבל return ולפיו ממשיך הלאה?

[dovid]

@shpro654 ראשית הקוד יפה, וניכר שאתה מכיר הרבה פיצ'רים בתכנות ובES6/TS בפרט.
אתחיל מהשאלה האחרוה, הדרך צריכה להיות מקום אחד שמטפל בהכל בעזרת פונקציות משנה שכמה שיותר ממוקדות בתפקיד מסויים וכמה שפחות ממשיכים את התהליך.
בא נראה את הקוד.
א. השורה הזו:

const getAds = () => {

שקולה (כמעט) לגמרי לפונקציה רגילה:

function getAds() {

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

ב. השורה הזו:

checkIfFolderChanghd(ads) && setAds(ads);

היא טריקית, לא שזה נורא, אבל אם זה לא חוסך אין סיבה שלא לכתוב ברחל בתך הקטנה:

    if(checkIfFolderChanghd(ads))
         setAds(ads);

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

function checkIfFolderChanghd(ads) {
    const adminAdsLength = adminSettings.adsLength
    let adsLength = ads.length;
    let newUpdate = '';
 
    if (!adsLength) return true;
 
    if (adsLength < adminAdsLength || adsLength > adminAdsLength) {
        return true;
    }
    
    for (let i = 0; i < adsLength; i++) {
        const { fileDateCreated } = ads[i];
        if (fileDateCreated > newUpdate) {
            newUpdate = fileDateCreated;
        }
    }
 
    if (newUpdate <= adminSettings.update) {
        return false;
    }
 
    return true;
}

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

if (adsLength != adminAdsLength)

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

for (let i = 0; i < adsLength; i++) {
    if (ads[i].fileDateCreated > adminSettings.update) 
        return true;
}

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

for (const ad of ads) {
    if (ad.fileDateCreated > adminSettings.update) 
        return true;
}

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

function checkIfFolderChanghd(ads) {
    return adminSettings.adsLength != ads.length ||
           ads.some(ad => ad.fileDateCreated > adminSettings.update)
}
 

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