2
\$\begingroup\$

Originally, I was going to post a question about how to condense four repetitive functions into a single function, but I wound up figuring it out myself before I could even post it.

However, the original idea that I had was to break down some of the repetitive sections into their own function and use those new sub-functions as part of the main one. I'm still not sure how I would have been able to make that work if I hadn't figured out the bigger picture.

function GetClock(){
    var d=new Date().addHours(1);
    var nday=d.getDay(),nmonth=d.getMonth(),ndate=d.getDate(),nyear=d.getYear();
    if(nyear<1000) nyear+=1900;
    var nhour=d.getHours(),nmin=d.getMinutes(),nsec=d.getSeconds(),ap;

    if(nhour===0){ap=" ";nhour=12;}
    else if(nhour<12){ap=" ";}
    else if(nhour===12){ap=" •";}
    else if(nhour>12){ap=" •";nhour-=12;}

    if(nhour<=9) nhour="0"+nhour;
    if(nmin<=9) nmin="0"+nmin;
    if(nsec<=9) nsec="0"+nsec;

    document.getElementById('clockbox').innerHTML=""+tday[nday]+", "+tmonth[nmonth]+" "+ndate+", "+nyear+" "+nhour+":"+nmin+":"+nsec+"<span class='ampm'>"+ap+"</span>";
}

This is one of the sections that I was originally thinking about splitting off into its own function:

if(nhour===0){ap=" ";nhour=12;}
else if(nhour<12){ap=" ";}
else if(nhour===12){ap=" •";}
else if(nhour>12){ap=" •";nhour-=12;}

These were my two feeble attempts to make it work:

function ampm() {
    if(nhour===0){ap=" ";nhour=12;}
    else if(nhour<12){ap=" ";}
    else if(nhour===12){ap=" •";}
    else if(nhour>12){ap=" •";nhour-=12;}
}

And then this:

function ampm(nhr,ampm) {
    if(nhr===0){ampm=" ";nhr=12;}
    else if(nhr<12){ampm=" ";}
    else if(nhr===12){ampm=" •";}
    else if(nhr>12){ampm=" •";nhr-=12;}
}

But they both failed due to (what I'm assuming are) issues with scope that I'm not sure how to solve.

\$\endgroup\$
2
\$\begingroup\$

Theres a lot of things here.

Disclaimer:

Using time stuff in JavaScript native is very hard and doesn't guarantee the cross browser. Also there are 300 of implementations of the same thing, plz don't make a new well. Please use a library like http://momentjs.com

First of all, is a little hard to read so, I started, formatting the code:

    function GetClock(){
        var d = new Date().addHours(1);
        var nday= d.getDay(),
            nmonth= d.getMonth(),
            ndate= d.getDate(),
            nyear= d.getYear();

        if(nyear<1000)
            nyear += 1900;

        var nhour= d.getHours(),
            nmin= d.getMinutes(),
            nsec= d.getSeconds(),
            ampm;

        if(nhour === 0) {
            ampm = " ";
            nhour = 12;
        } else if (nhour<12) {
            amPmSymbol =  " ";
        } else if(nhour===12) {
            ampm= " •";
        } else if(nhour>12) {
            ampm = " •";
            nhour-= 12;
        }

        if(nhour<=9)
            nhour = "0" + nhour;

        if(nmin<=9)
            nmin = "0" + nmin;

        if(nsec<=9)
            nsec = "0" + nsec;

        document.getElementById('clockbox').innerHTML =
            tday[nday] + ", " +
            tmonth[nmonth] + " " +
            ndate+", " +
            nyear + " " +
            nhour + ":" +
            nmin + ":" +
            nsec + "<span class='ampm'>" + ampm + "</span>";
    }

So, i can see some issues and repeated code on this.

  • This function doesn't return anything. So, why is called as Get?, I recommend use updateTime
  • I can split date format, from hour/time format and am/pm symbol (I guess).
  • also, I eliminate the unused/usefulness variables from the clock creation. Now all additional functions return strings, so, can be concatenate with the template.

    function updateTime(){
        var aDate = new Date().addHours(1);
        var ndate = aDate.getDate(),
            nyear = aDate.getYear() ? ;
    
        if(nyear<1000)
            nyear += 1900;
    
        document.getElementById('clockbox').innerHTML =
            tday[aDate.getDay()] + ", " +
            tmonth[aDate.getMonth()] + " " +
            ndate + ", " +
            nyear + " " + getTimeAsFormatString(aDate);
    }
    
    function getTimeAsFormatString(aDate) {
        var hour = aDate.getHours(),
            mins = aDate.getMinutes(),
            secs = aDate.getSeconds();
    
        if(hour === 0) {
            hour = 12;
        } else if(hour>12) {
            hour-= 12;
        }
    
        hour = hour<= 9  ? "0" + hour : hour;
        mins = mins<= 9  ? "0" + mins : mins;
        secs = secs <= 9  ? "0" + secs : secs;
    
        return hour + ":" + mins  + ":" + secs +  "<span class='ampm'>" + getAmPmSymbol(hour) + "</span>";
    
    }
    
    function getAmPmSymbol(hour) {
        return (hour >= 12) ? " •" : " ";
    }
    
\$\endgroup\$
  • \$\begingroup\$ "Create"? What is it creating? The clockbox element already exists; the function is just updating its text content. \$\endgroup\$ – 200_success Nov 7 '15 at 7:21
1
\$\begingroup\$

I don't see a need for a new function here just some simplification, unless you plan on reusing these smaller blocks.

that said here is an example:

if(nhour===0){ap=" ";nhour=12;}
else if(nhour<12){ap=" ";}
else if(nhour===12){ap=" •";}
else if(nhour>12){ap=" •";nhour-=12;}

Can be replaced with the following:

ap = nhour >= 12 ? '  •' : '  ';
nhour = nhour%12 ? nhour%12 : 12;

Also you attempts at functionalizing lacked returns. This isn't necessarily wrong but lack of scope make it hard to know

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.