current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I'm doing a clock in javascript and I am not sure this way is the best way to rotate the pointers.

Is this ok, or is there a better way?

// browser vendors
var browsers = [
    'webkit',
    'Moz',
    'ms',
    'O',
    ''];

// function to move the pointers
function moveMe(el, unit) {
    var deg = unit * 360 / 60;
    browsers.each(function (b) {
        el.style[b + 'Transform'] = 'rotate(' + deg + 'deg)';
    });
}

// function to check the time
function checkTime() {
    var date = new Date();
    var seconds = date.getSeconds();
    var minutes = date.getMinutes();
    var hours = date.getHours();
    moveMe(sPointer, seconds);
    !seconds && moveMe(mPointer, minutes);
    !minutes && moveMe(hPointer, hours);
}

The fiddle I'm playing with is here.

share|improve this question
up vote 2 down vote

First of all, your code use Array.each() method, which is a non-standard Javascript method, added by Mootools. I would rather use a standard loop, in order to keep a vanilla JS code.

Then, instead of setting all properties every time, you should test which properties are supported by the browser and set them accordingly.

Finally, since you are moving minutes only when !seconds (i.e. seconds == 0), the clock initialization is a bit weird. On page load, you should always move minutes and hours.

share|improve this answer
    
Thanks for feedback! I do initialization on my fiddle. About the .each() I have more mootools but sure, a vanilla loop is better. Good idea to detect browser... About the rotation itself, is that the best way to do it? By setting style on the image? – Rikard Jan 11 '14 at 18:46
    
That's indeed the most straightforward solution. Otherwise, you'll need to use HTML5 Canvas API. – Guillaume Poussel Jan 11 '14 at 18:50
    
For reference, there is an Array.forEach() method that is standard JavaScript (ES 5.1). – rink.attendant.6 Jan 12 '14 at 6:51
    
I disagree with the .each comment. If you're using a framework you can write code for the framework – megawac Feb 8 '14 at 23:35
up vote 1 down vote

In GCC, if you write !seconds && moveMe(mPointer, minutes); and compile with -Wall you get this warning:

[Warning] value computed is not used [-Wunused-value]

I know this is not C, but the problem is the same and I can't see why you don't explicitly write if.

share|improve this answer
    
+1 for pointing out a possible bug. Is this also the case if the first value is not negated with !? Because this is a common practise in javascript – Rikard Jan 13 '14 at 22:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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