3
\$\begingroup\$

I want to write a function which returns true if a selected date is smaller than the current date, else return false.

This function is working as expected but I want to know the best possible way to do this.

private boolean isValidDate() {

    if(year<yearCurr)
    {
        //selected year is small date is small
        return true;
    }else
        if(year>yearCurr)
        {
            //selected year is greater date is greater
            return false;
        }else
        {
            //selected year is same as current year check month

            if(month<monthCurr)
            {
                //selected month is small date is small   
                return true;
            }else
                if(month>monthCurr)
                {
                    //if selected month is greater date is greater
                    return false;
                }
                else
                {
                    // selected day of month is less than or equal to current date; date is valid 
                    if(day<=dayCurr)
                        return true;
                    else
                        return false;
                }
        }
}
\$\endgroup\$
5
\$\begingroup\$

Another more compact way of organizing the conditionals:

if (yearCurr != year) {
    return year < yearCurr;
}
if (monthCurr != month) {
    return month < monthCurr;
}
return day <= dayCurr;
\$\endgroup\$
6
\$\begingroup\$

Just wanted to chime in:

  • Nobody mentioned this, but isValidDate() to determine whether 1 date is earlier than another date is frankly terrible. Call it isEarlierDate()
  • Where are the variables? Are those variables all defined outside of this function. That does not look good.
  • If there is one thing you learn today it is that this

    if(day<=dayCurr)
        return true;
    else
        return false;
    

    should always be written as

    return day<=dayCurr
    
  • As per @Heslacher, write out your variable names

This is my over-commented counter-proposal:

function isEarlierDate(){

  return year < currentYear || //Last year is in the past
         year == currentYear && month < currentMonth || //This year, earlier month is in the past
         year == currentYear && month == currentMonth && day < currentDay || //..
         false; //This is clearly not an earlier date
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Glad that you brought up the function name and where the heck the variables are coming from. I don't really agree without your counter-proposal only because complex booleans like that are harder to maintain (imo) because they're harder to read. I had a project where I used concatenated booleans using short-circuiting that I scrapped completely when I refactored, simply because it took me (the author) more than 10 minutes to figure out what the hell I had previously coded (4 months earlier). \$\endgroup\$ – Chris Cirefice Dec 5 '14 at 22:45
  • \$\begingroup\$ Agreed in general, but if this function would take me 10 minute to figure out, then I would retire ;) \$\endgroup\$ – konijn Dec 8 '14 at 13:03
  • \$\begingroup\$ it's definitely probable that someone like me (junior programmer) would be maintaining your code. It's about the next guy who has to read it. I wouldn't be happy if it were. I don't know about other languages, but you can declare conditionals and assign to variable in JavaScript. Naming conditions, if they're complex, and concatenating with || would make it at least slightly more readable. \$\endgroup\$ – Chris Cirefice Dec 8 '14 at 13:56
  • \$\begingroup\$ Interesting. Just to be sure, are you saying that as a junior progamming my snippet is too complex / too hard to maintain? \$\endgroup\$ – konijn Dec 8 '14 at 13:59
  • \$\begingroup\$ Not for me, no. But you wouldn't believe how many of my senior classmates would take 15 minutes to understand that boolean. My current job works with Google Calendar a lot so I'n familiar with this type of code, but a lot if my classmates would be confused at such a boolean, especially if it wasn't such an intuitive one. By reason, you can figure out what aspects of a date need to be examined to determine if one comes before another. In other applications, it might be inherently harder to understand. \$\endgroup\$ – Chris Cirefice Dec 8 '14 at 14:07
3
\$\begingroup\$

There are a number of common tricks to solving this problem. The most logical one in Java 8 would be to use a LocalDate instance....

return LocalDate.of(year, month, day).isBefore(LocalDate.of(yearCurr, monthCurr, dayCurr));

Similar things can be done with a Date, or Calendar instance from earlier Java versions.

For a real simple 'hack', though, you can also do....

return (year * 10000 + month * 100 + day) < (yearCurr * 10000 + monthCurr * 100 + dayCurr);

Regardless of what system you use, you should still heed @Heslacher's suggestions about naming, and conditionals.

\$\endgroup\$
2
\$\begingroup\$
  • You shouldn't shorten variable names. e.g dayCurr should be either currentDay or dayCurrent

  • As you are returning if a if condition evaluates to true, you can remove the else

This

if (condition) {
    return true;
} else {
    return false;
}

can be rewritten to

return condition;

So your former method could look like

private boolean isValidDate() {
    if (year < yearCurr) {
        return true;
    }
    if (year > yearCurr) {
        return false;
    }

    if (month < monthCurr) {
        return true;
    }
    if (month > monthCurr) {
        return false;
    }
    return day <= dayCurr;
}  
\$\endgroup\$
2
\$\begingroup\$

This example is why you should ALWAYS use brackets around your conditional statements in a language like Java. In case the code from the link breaks one day: this code is from Apple's SSL/TLS security threat bug that was identified in Feb 2014.

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;
    ...

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail; // THIS BREAKS THE CODE!!!
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        goto fail;
    ...

fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}

Most professors and professionals I've come into contact with agree that it's generally bad practice to have multiple return statements in a single function. The reason I follow this standard is for debugging purposes - I don't have to leave the function to see where the return value is adopting an unexpected value when I'm in the debugger, this way.

When formatting your code, try to minimize the need for indentation - it makes the code easier to skim over without losing the gist of what it's doing.

When working with boolean values, you can often avoid extra control statements and nesting by simply returning evaluated boolean expressions.

Boolean checks are one of the fastest things you can do in code. I don't think you need to be concerned about performance on this particular block of logic.

private boolean isValidDate() {
    // It's always good to initialize your variables
    boolean returnVal = false;

    if (year == yearCurr) {
        //selected year is same as current year check month
        if (month == monthCurr) {
            // selected month is same as current month check day
            returnVal = day <= dayCurr;   
        } else {
            returnVal = month < monthCurr;
        }
    } else {
        returnVal = year < yearCurr;
    }  

    return returnVal;
}

Using the ternary operator

private boolean isValidDate() {
    // It's always good to initialize your variables
    boolean returnVal = false;

    if (year == yearCurr) {
        //selected year is same as current year check month
        returnVal = (month == monthCurr) ? day <= dayCurr : month < monthCurr; 
    } else {
        returnVal = year < yearCurr;
    }  

    return returnVal;
}

Going a little overboard

private boolean isValidDate() {    
    return year == yearCurr
           ? (month == monthCurr ? day <= dayCurr : month < monthCurr)
           : year < yearCurr;
}

But, maybe not THAT overboard: https://codereview.stackexchange.com/questions/10466/is-using-the-ternary-operator-like-this-considered-less-readable

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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