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

• 200_success: "Create"? What is it creating? The clockbox element already exists; the function is just updating its text content.

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