Skip to main content
Code Review

Return to Question

Replaced original code with updated code since it was added before answers
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

Countdown script in javascriptJavaScript

Here is the code:

(function() {
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 pad = function(number) { return (number < 10 ? '0' : '') + number }
 ;(function() {
 curDate = new Date()
 diff = new Date(curDate.getTime() - endDate.getTime())
 days = Math.abs(diff / (24 * 60 * 60 * 1000))
 tmp = diff % (24 * 60 * 60 * 1000)
 hours = Math.abs(tmp / (60 * 60 * 1000))
 tmp = tmp % (60 * 60 * 1000)
 minutes = Math.abs(tmp / (60 * 1000))
 tmp = tmp % (60 * 1000)
 seconds = Math.abs(tmp / 1000)
 countdown = pad(parseInt(days, 10)) + ':' + pad(parseInt(hours, 10)) + ':' + pad(parseInt(minutes, 10)) + ':' + pad(parseInt(seconds, 10))
 if ('textContent' in el) el.textContent = countdown
 else el.innerText = countdown
 setTimeout(arguments.callee, 1000)
 }())
}())

PS: about the semicolons missing, it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

Edit : @Zirak has reviewed my code. Here is the reviewed version, which does look way cleaner!

(function() {
 //the variable block here seems kinda weird, but whatever toots your horn
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 //added Math.floor. this is already a shortcut, might as well make it a double one
 pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },
 
 //calculate these constants once, instead of over and over
 minute = 60 * 1000,
 hour = minute * 60,
 day = hour * 24
 
 ;(function tick() {
 curDate = new Date()
 //you want the absolute value of this, not of individual calculations using this
 diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
 days = diff / day
 
 tmp = diff % day
 hours = tmp / hour
 
 tmp = tmp % hour
 minutes = tmp / minute
 
 tmp = tmp % minute
 seconds = tmp / 1000
 
 //parseInt was redundant
 countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)
 
 if ( 'textContent' in el ) {
 el.textContent = countdown
 }
 else {
 el.innerText = countdown
 }
 
 //dont't use arguments.calle, it's deprecated
 //simply use a named function expression
 setTimeout(tick, 1000)
 }())
}())​

About the semicolons missing: it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

Countdown in javascript

Here is the code:

(function() {
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 pad = function(number) { return (number < 10 ? '0' : '') + number }
 ;(function() {
 curDate = new Date()
 diff = new Date(curDate.getTime() - endDate.getTime())
 days = Math.abs(diff / (24 * 60 * 60 * 1000))
 tmp = diff % (24 * 60 * 60 * 1000)
 hours = Math.abs(tmp / (60 * 60 * 1000))
 tmp = tmp % (60 * 60 * 1000)
 minutes = Math.abs(tmp / (60 * 1000))
 tmp = tmp % (60 * 1000)
 seconds = Math.abs(tmp / 1000)
 countdown = pad(parseInt(days, 10)) + ':' + pad(parseInt(hours, 10)) + ':' + pad(parseInt(minutes, 10)) + ':' + pad(parseInt(seconds, 10))
 if ('textContent' in el) el.textContent = countdown
 else el.innerText = countdown
 setTimeout(arguments.callee, 1000)
 }())
}())

PS: about the semicolons missing, it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

Edit : @Zirak has reviewed my code. Here is the reviewed version, which does look way cleaner!

(function() {
 //the variable block here seems kinda weird, but whatever toots your horn
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 //added Math.floor. this is already a shortcut, might as well make it a double one
 pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },
 
 //calculate these constants once, instead of over and over
 minute = 60 * 1000,
 hour = minute * 60,
 day = hour * 24
 
 ;(function tick() {
 curDate = new Date()
 //you want the absolute value of this, not of individual calculations using this
 diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
 days = diff / day
 
 tmp = diff % day
 hours = tmp / hour
 
 tmp = tmp % hour
 minutes = tmp / minute
 
 tmp = tmp % minute
 seconds = tmp / 1000
 
 //parseInt was redundant
 countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)
 
 if ( 'textContent' in el ) {
 el.textContent = countdown
 }
 else {
 el.innerText = countdown
 }
 
 //dont't use arguments.calle, it's deprecated
 //simply use a named function expression
 setTimeout(tick, 1000)
 }())
}())​

Countdown script in JavaScript

(function() {
 //the variable block here seems kinda weird, but whatever toots your horn
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 //added Math.floor. this is already a shortcut, might as well make it a double one
 pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },
 
 //calculate these constants once, instead of over and over
 minute = 60 * 1000,
 hour = minute * 60,
 day = hour * 24
 
 ;(function tick() {
 curDate = new Date()
 //you want the absolute value of this, not of individual calculations using this
 diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
 days = diff / day
 
 tmp = diff % day
 hours = tmp / hour
 
 tmp = tmp % hour
 minutes = tmp / minute
 
 tmp = tmp % minute
 seconds = tmp / 1000
 
 //parseInt was redundant
 countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)
 
 if ( 'textContent' in el ) {
 el.textContent = countdown
 }
 else {
 el.innerText = countdown
 }
 
 //dont't use arguments.calle, it's deprecated
 //simply use a named function expression
 setTimeout(tick, 1000)
 }())
}())​

About the semicolons missing: it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

Tweeted twitter.com/#!/StackCodeReview/status/183246783294603264
added 1935 characters in body
Source Link

Edit : @Zirak has reviewed my code. Here is the reviewed version, which does look way cleaner!

(function() {
 //the variable block here seems kinda weird, but whatever toots your horn
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 //added Math.floor. this is already a shortcut, might as well make it a double one
 pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },
 
 //calculate these constants once, instead of over and over
 minute = 60 * 1000,
 hour = minute * 60,
 day = hour * 24
 
 ;(function tick() {
 curDate = new Date()
 //you want the absolute value of this, not of individual calculations using this
 diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
 days = diff / day
 
 tmp = diff % day
 hours = tmp / hour
 
 tmp = tmp % hour
 minutes = tmp / minute
 
 tmp = tmp % minute
 seconds = tmp / 1000
 
 //parseInt was redundant
 countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)
 
 if ( 'textContent' in el ) {
 el.textContent = countdown
 }
 else {
 el.innerText = countdown
 }
 
 //dont't use arguments.calle, it's deprecated
 //simply use a named function expression
 setTimeout(tick, 1000)
 }())
}())​

Edit : @Zirak has reviewed my code. Here is the reviewed version, which does look way cleaner!

(function() {
 //the variable block here seems kinda weird, but whatever toots your horn
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 //added Math.floor. this is already a shortcut, might as well make it a double one
 pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },
 
 //calculate these constants once, instead of over and over
 minute = 60 * 1000,
 hour = minute * 60,
 day = hour * 24
 
 ;(function tick() {
 curDate = new Date()
 //you want the absolute value of this, not of individual calculations using this
 diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
 days = diff / day
 
 tmp = diff % day
 hours = tmp / hour
 
 tmp = tmp % hour
 minutes = tmp / minute
 
 tmp = tmp % minute
 seconds = tmp / 1000
 
 //parseInt was redundant
 countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)
 
 if ( 'textContent' in el ) {
 el.textContent = countdown
 }
 else {
 el.innerText = countdown
 }
 
 //dont't use arguments.calle, it's deprecated
 //simply use a named function expression
 setTimeout(tick, 1000)
 }())
}())​
added 75 characters in body
Source Link

I've written this little countdown scriptlittle countdown script to count until the end of our school days.

Now, that was written quickly, badly remembering modulo from a few years ago, and I don't think that's very optimized.

I thought about updating only the seconds and check if it needs to update the minute too, etc. But it wouldn't be accurate as setTimeout is not (depending if the browser lags, etc).

Here is the code:

(function() {
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 pad = function(number) { return (number < 10 ? '0' : '') + number }
 ;(function() {
 curDate = new Date()
 diff = new Date(curDate.getTime() - endDate.getTime())
 days = Math.abs(diff / (24 * 60 * 60 * 1000))
 tmp = diff % (24 * 60 * 60 * 1000)
 hours = Math.abs(tmp / (60 * 60 * 1000))
 tmp = tmp % (60 * 60 * 1000)
 minutes = Math.abs(tmp / (60 * 1000))
 tmp = tmp % (60 * 1000)
 seconds = Math.abs(tmp / 1000)
 countdown = pad(parseInt(days, 10)) + ':' + pad(parseInt(hours, 10)) + ':' + pad(parseInt(minutes, 10)) + ':' + pad(parseInt(seconds, 10))
 if ('textContent' in el) el.textContent = countdown
 else el.innerText = countdown
 setTimeout(arguments.callee, 1000)
 }())
}())

PS: about the semicolons missing, it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

I've written this little countdown script to count until the end of our school days.

Now, that was written quickly, badly remembering modulo from a few years ago, and I don't think that's very optimized.

I thought about updating only the seconds and check if it needs to update the minute too, etc. But it wouldn't be accurate as setTimeout is not (depending if the browser lags, etc).

Here is the code:

(function() {
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 pad = function(number) { return (number < 10 ? '0' : '') + number }
 ;(function() {
 curDate = new Date()
 diff = new Date(curDate.getTime() - endDate.getTime())
 days = Math.abs(diff / (24 * 60 * 60 * 1000))
 tmp = diff % (24 * 60 * 60 * 1000)
 hours = Math.abs(tmp / (60 * 60 * 1000))
 tmp = tmp % (60 * 60 * 1000)
 minutes = Math.abs(tmp / (60 * 1000))
 tmp = tmp % (60 * 1000)
 seconds = Math.abs(tmp / 1000)
 countdown = pad(parseInt(days, 10)) + ':' + pad(parseInt(hours, 10)) + ':' + pad(parseInt(minutes, 10)) + ':' + pad(parseInt(seconds, 10))
 if ('textContent' in el) el.textContent = countdown
 else el.innerText = countdown
 setTimeout(arguments.callee, 1000)
 }())
}())

PS: about the semicolons missing, it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

I've written this little countdown script to count until the end of our school days.

Now, that was written quickly, badly remembering modulo from a few years ago, and I don't think that's very optimized.

I thought about updating only the seconds and check if it needs to update the minute too, etc. But it wouldn't be accurate as setTimeout is not (depending if the browser lags, etc).

Here is the code:

(function() {
 var el = document.getElementById('countdown'),
 endDate = new Date('March 30, 2012 18:10:00'),
 curDate,
 diff,
 days,
 hours,
 minutes,
 seconds,
 tmp,
 countdown,
 pad = function(number) { return (number < 10 ? '0' : '') + number }
 ;(function() {
 curDate = new Date()
 diff = new Date(curDate.getTime() - endDate.getTime())
 days = Math.abs(diff / (24 * 60 * 60 * 1000))
 tmp = diff % (24 * 60 * 60 * 1000)
 hours = Math.abs(tmp / (60 * 60 * 1000))
 tmp = tmp % (60 * 60 * 1000)
 minutes = Math.abs(tmp / (60 * 1000))
 tmp = tmp % (60 * 1000)
 seconds = Math.abs(tmp / 1000)
 countdown = pad(parseInt(days, 10)) + ':' + pad(parseInt(hours, 10)) + ':' + pad(parseInt(minutes, 10)) + ':' + pad(parseInt(seconds, 10))
 if ('textContent' in el) el.textContent = countdown
 else el.innerText = countdown
 setTimeout(arguments.callee, 1000)
 }())
}())

PS: about the semicolons missing, it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

added 1123 characters in body
Source Link
Loading
Source Link
Loading
default

AltStyle によって変換されたページ (->オリジナル) /