Wątek przeniesiony 2015-10-13 20:38 z Webmastering przez dzek69.

Prośba o ocenę krótkiego kodu.

0

Cześć. Miałem stworzyć mały skrypt, którego zadaniem jest wyświetlanie użytkownikowi dialogu z tabelką o nadchodzących wydarzeniach dla niego. Pierwszy dialog miał się wyświetlać raz per login i obejmować wydarzenia na nadchodzący dzień, a drugi dialog miał pokazywać wydażenia które odbędą się w < 15 minut. Oczywiście odstęp pomiędzy dialogami to minimalnei 15 min. tzn. nie spamuje nam to okienko co chwilę, tylko zbiorczo pokazuje na nadchodzace 15 minut (jezeli takie wydazenia sa oczywiscie). Po wylogowaniu wszystko się resetuje.

Oto kod, sama część JS, bo głownie na tej części mi zależy. Dziękuję bardzo za poświęcony czas :)

var upcomingEvents = {
        init: function (options) {
            this.bindHandlers();
            var o = this.options = options;

            this.checkForEventsCount(o.searchParams, this.displayDialog, o.makeDialogParams);
        },
        bindHandlers: function () {
            $("[data-rack='logout']").click($.proxy(function () {
                this.discardLocalStorageInfo();
            }, this));
        },
        checkForEventsCount: function(search, callback, params) {
            $.ajax({
                type: "POST",
                url: "/activity/index/get-upcoming-events-count",
                data: search,
                success: function(msg){
                    if(msg !== "0") {
                        callback.call(upcomingEvents, params);
                    }
                }
            });
        },
        displayDialog: function (params) {
            if(typeof params === "undefined") params = arguments;

            var obj = $('<a></a>'), search;

            if (typeof params.rel !== "undefined") {
                obj.attr("rel", params.rel);
            }

            if (typeof this.options.searchParams !== "undefined") {
                search = JSON.stringify(this.options.searchParams);
                obj.attr("data-params", search);
            }

            if(typeof params["data-close"] !== 'undefined')
                obj.attr("data-close", params["data-close"]);

            if (typeof params.url !== "undefined") {
                obj.attr('data-href', params.url);
                makeDialogGeneric(null, obj);
                this.setLocalStorageInfo(this.options.dialogType, true);
                this.setNextNotificationGap();
            }
        },
        ifDisplayDialog: function() {
            return this.getLocalStorageInfo().length === 0
        },
        ifDisplayDialogLastMinute: function() {
            var ifRegularAlreadyDisplayed = this.getLocalStorageInfo().length === 2;
            var ifGapHasPassed = this.getNotificationGap() < new Date().getTime();
            return ifRegularAlreadyDisplayed && ifGapHasPassed;
        },
        getLocalStorageInfo: function () {
            var patt = /upcoming_events/;
            var keys = [];
            for(var key in localStorage) {
                if(localStorage.hasOwnProperty(key) && patt.test(key))
                    keys.push(key);
            }
            return keys;
        },
        setLocalStorageInfo: function (name, value) {
            localStorage.setItem(name, value);
        },
        discardLocalStorageInfo: function () {
            var patt = /upcoming_events/;
            for(var key in localStorage) {
                if(localStorage.hasOwnProperty(key) && patt.test(key))
                    localStorage.removeItem(key);
            }
        },
        setNextNotificationGap: function(gap) {
            gap = gap || new Date().getTime() + locale.time.FIFTEEN_MINUTES_IN_MILLISECONDS;

            localStorage.setItem("upcoming_events_next", gap);
        },
        getNotificationGap: function() {
            return localStorage.getItem("upcoming_events_next");
        }
    };

    (function() {
        var currentTime = new Date().getTime();
        var endDate, dialogType;

        if (upcomingEvents.ifDisplayDialog()) {
            endDate = currentTime + locale.time.DAY_IN_MILLISECONDS;
            dialogType = "upcoming_events"
        } else if (upcomingEvents.ifDisplayDialogLastMinute()) {
            endDate = currentTime + locale.time.FIFTEEN_MINUTES_IN_MILLISECONDS;
            dialogType = "upcoming_events_last_minute"
        }

        if(typeof endDate !== "undefined") {
            upcomingEvents.init({
                searchParams: {
                    search: { // got to be nested object to access it by 'search' in controller
                        start_date: currentTime,
                        end_date: endDate
                    },
                    sort: "start_dateDes"
                },
                makeDialogParams: {
                    rel: "800, 400",
                    url: "/activity/index/upcoming-events/",
                    "data-close": true
                },
                dialogType: dialogType
            });
        } else {
            $("[data-rack='logout']").click($.proxy(function () {
                this.discardLocalStorageInfo();
            }, upcomingEvents));
        }
    })();
0
  • ifDisplayDialog brzmi nieciekawie, canDisplayDialog czy shouldDisplayDialog etc. lepiej się sprawdzi.
  • dlaczego od razu last minute, a nie pokombinujesz ze słówkiem kwadrans?
  • personalnie lubię oddzielać kontrolery ajaksowe od reszty, czyli zamiast /activity/index/get-upcoming-events-count wykorzystuję /activity/ajax/get-upcoming-events-count
  • skoro jeden request to get-upcoming-events-count, to dlaczego drugi to upcoming-events, a nie get-upcoming-events?

Oprócz tego nic mi się tak bardziej w oczy nie rzuciło.

0

Dzięki za opinie :) O tym ifie gdzies przeczytałem, że zmienne tego typu powinny byc w takiej konwencji, ale moze to glupota. Jezeli chodzi o ajax to moze i racja, ale u mnie w projekcie juz tak jest i raczej nie uda mi sie tego zmienic, a upcoming-event to nazwa akcji, stad nie ma tam slowa get.

0
Złoty Mleczarz napisał(a):

Dzięki za opinie :) O tym ifie gdzies przeczytałem, że zmienne tego typu powinny byc w takiej konwencji, ale moze to glupota.

Może za szybko spojrzałeś i tam nie było if tylko is. Nie wiem jak jest w JSie, ale pewnie podobnie jak w innych językach, w których gettery zmiennych typu boolean nazywamy z przedrostkiem is zamiast get. Więc może tutaj lepszą nazwą niż

ifDisplayDialog  

byłaby isDisplayDialog

1
  • masz duplikację kodu w metodach getLocalStorageInfo oraz discardLocalStorageInfo. To się powtarza:

var patt = /upcoming_events/;
(...)
for(var key in localStorage) {
if(localStorage.hasOwnProperty(key) && patt.test(key))

Można uniknąć tej duplikacji np. poprzez wydzielienie trzeciej funkcji, z której będą korzystać pierwsze dwie.

  • stworzyłeś jeden obiekt, w którym mieszasz ileś zagadnień (łamiąc zasadę single responsibility), poprzez wrzucanie naraz kodu:
  • odwołującego się do DOM (np. w metodzie displayDialog)
  • odwołującego się do AJAXa
  • zapisującego w localStorage
  • oraz wreszcie kodu, który zlicza czas

Jeśli to jest mały skrypt, który po prostu działa, to jeszcze takie mieszanie wszystkiego ujdzie, ale jak będziesz to rozbudował albo używał jako część większej aplikacji, to w końcu nadejdzie ten moment, kiedy ten piękny "god object" ci się rozleci przy pierwszej próbie modyfikacji kodu.

Bardziej eleganckie rozwiązanie polegałoby tworzyłoby kilka osobnych podobiektów, które byłyby odpowiedzialne za jedną rzecz (np. za wyliczenie czasu, albo za wyświetlenie formularza), oraz obiektu, który by korzystał z tych podobiektów łącząc je w całość.

0

Wielkie dzieki za super analize mojego kodu :) Generalnie sa jakies moje pierwsze wypociny w js'ie, wiec staram sie to wszystko jakos zkleic. Wezme ten sam kod i w oparciu o to co napisałeś przepiszę ten skrypt.

0

A jeszcze przepraszam, że duplikuję posty, ale mam pytanie odnośnie tych kilku obiektów. Ogólnie u nas w projekcie jest straszny burdel, ale gdybym chciał to zrobić poprawnie to gdzie powinien wylądować taki skrypt? Jako kolejny "dokument redi" w pliku common.js, czy może w takim wypadku powinno się stowrzyć na każdy taki skrypt osobny plik? Wtedy organizacja by była cirka coś takiego:
plik upcoming-events.js

var obiektOdpZaCzas .... 

var obiektOdpZaLocalStorage..

var obiektOdpowiedzialnyZaDialog

var controller(?)LaczacyWszystko

0

czy może w takim wypadku powinno się stowrzyć na każdy taki skrypt osobny plik?

Tak, lepiej podzielić na różne pliki. Najlepiej też powsadzać to w jakieś moduły i użyć jakiegoś narzędzia do tworzenia buildów(np. Browserify), które połączy te różne pliki w całość.

0

Ekstra, dzieki za poswiecony czas.

1 użytkowników online, w tym zalogowanych: 0, gości: 1