HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Revealing store sales based on time

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
revealingtimestorebasedsales

Problem

I am delving into trying to write some JavaScript code.
I understand the basics, (the very basics...) enough that I have been able to write the following and it works.

However, my knowledge doesn't stretch enough to be able to work out if this was the "best" way to write what I am trying to achieve. I don't want to just settle on it being "done" because it works, if there is a way to improve it and in turn, improve my understanding, I'd like to learn from people on best practices.

Currently, the code does the following:

If a user comes to the page before Sales Week, a message will display to the user telling them the days/hours/minutes until SalesWeek

(Please note that you won't be able to actually see the countdown at present on the code provided as it's a different piece of JS which is why it's in a funny span but it was written by a predecessor and I know it works.)

If a user comes to the page during sales week it will show which daily deal is open (and how long there is until it expires, using the same countdown timer as above which sadly is not included in this code but does work) and which deals have expired with a content div below for the actual sale items.

If the user comes to the page AFTER sales week they get a message to say that all the sales are over.

As a caveat, I know that using the client date is not ideal, and that server time should be used.
Also I think this could have all been created in the back-end (where the deals couldn't be found out by someone looking at the source) but again, this isn't realistic for right now.

I'd really appreciate any guidance on my javascript itself, in the way I have formed it, and if there are better ways to achieve the same result.

Here is a demo on JSFiddle.

```
var date=new Date();
var year=date.getFullYear();
var month=date.getMonth();
var day=date.getDate();
var hours=date.getHours();
var minutes=date.getMinutes();

function SetDivContent() {

if ((year == 2015 && month = 28) {
cons

Solution

Because of the way that if-else statements work, you can eliminate redundancies.

Here's the code cut up some to focus on just the if-else statements:

if ((year == 2015 && month = 28) { 
}
else {
    if ( (day=="23" && hours>="01") || (day=="24" && hours="01") || (day=="25" && hours="01") || (day=="26" && hours="01") || (day=="27" && hours="01") || (day=="28" && hours<="00") ){
    }
}


The first statement first checks the leftmost set of parentheses, then the next one, etc. But these all have one thing in common: year == 2015. We can extract that so it doesn't check it so much.

if (year == 2015 && (month = 28)) { 
    }


Now, this portion will evaluate the same way, but it's drier, will only check year a maximum of twice (instead of five before) and easier to edit later.

You can also extract the strings "bem-sale-week--item-expired" & "bem-sale-week--item-open" into vars, up with the rest of your variable declarations:

var date = new Date();
var year = date.getFullYear(),
    month = date.getMonth(),
    day = date.getDate(),
    hours = date.getHours(),
    minutes = date.getMinutes(),
    ITEM_EXPIRED = "bem-sale-week--item-expired",
    ITEM_OPEN = "bem-sale-week--item-open";


I added whitespace around the equals sign. I also changed the declarations to use the comma format and indentation (except for var date, which has to initialize before year, month, et al can call its methods). This isn't super important here, but it's good to know. Try to use the comma format particularly when you are repeating it a lot, like in a loop, as each call to "var" is a little work.

And then another thing that's good is comments!

// if we're in october 2015 before the 23rd:
if (year == 2015 && (month = 28)) {
// ...
}
// else: we want to specify actions for each of the sale days (23rd thru 27th)
else {
// ...

Hope this helps!

(P.S.: I don't know what behavior you want for 2016 and later, but right now it might be unaccounted for!)

Code Snippets

if ((year == 2015 && month < 10) || (year == 2015 && month == 10 && day < 23) || (year == 2015 && month == 10 && day == 23 && hours < 01)) { 
// do things
}
else if (year == 2015 && month == 10 && day >= 28) { 
}
else {
    if ( (day=="23" && hours>="01") || (day=="24" && hours<="00") ) {

    }
    else if ( (day=="24" && hours>="01") || (day=="25" && hours<="00") ){

    }
    else if ( (day=="25" && hours>="01") || (day=="26" && hours<="00") ){
    }
    else if( (day=="26" && hours>="01") || (day=="27" && hours<="00") ){
    }
    else if( (day=="27" && hours>="01") || (day=="28" && hours<="00") ){
    }
}
if (year == 2015 && (month < 10 //indentation can help readability
                        || (month == 10 && day < 23) //or not! it's just preference
                        || (month == 10 && day == 23 && hours < 01))
    ) { 
    // do things
    }
    // altering this statement too
    else if (year == 2015 && (month == 10 && day >= 28)) { 
    }
var date = new Date();
var year = date.getFullYear(),
    month = date.getMonth(),
    day = date.getDate(),
    hours = date.getHours(),
    minutes = date.getMinutes(),
    ITEM_EXPIRED = "bem-sale-week--item-expired",
    ITEM_OPEN = "bem-sale-week--item-open";

Context

StackExchange Code Review Q#105391, answer score: 3

Revisions (0)

No revisions yet.