Updating a clock display with the time

Question

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.

Answer 1

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) ? " •" : " ";
  }
  

Answer 2

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