• Post Reply Bookmark Topic Watch Topic
  • New Topic

Performance and improvement  RSS feed

 
Greenhorn
Posts: 17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi guys,

I wrote a function to determine if a year is leap or not. I would like to hear any comments on how to improve it and why. I am new programmer and I just trying to get better at it.

I tested it and it provided the correct answer in all tests.



I was undecided between using a case/break or these if loops and obviously I chose the if loops.

Thanks,

Richard.
 
Sheriff
Posts: 22846
43
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One way to improve code is to leave out unnecessary code. For example this:



works exactly the same as this:



(That's because if "year" is divisible by 400, it's automatically also divisible by 100 and by 4, so the other two conditions are redundant.)

You might think that it doesn't matter which of those two you use, since they both do the same thing. But when another programmer has to look at the code, they are going to have to stop and ask themselves why you did it the hard way. After a while they would realize, as I did, that your code works fine, but it's still a waste of that programmer's time.
 
author
Sheriff
Posts: 23295
125
C++ Chrome Eclipse IDE Firefox Browser Java jQuery Linux VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Not sure of the purpose of this part...

Richard Innamorato wrote:



If a number is evenly divisible by 400, then isn't it automatically evenly divisible by 100 and evenly divisible by 4 too... meaning can't you just code it like this instead...



[EDIT: Wow... beaten to the answer by one second !! ]

Henry
 
Henry Wong
author
Sheriff
Posts: 23295
125
C++ Chrome Eclipse IDE Firefox Browser Java jQuery Linux VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

BTW, you can also make the code less verbose, like this...



Henry
 
Bartender
Posts: 689
17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Personally I would probably have written it something like this:



This nicely spells out the two conditions that make a year a leap year, in readable text. It also allows a reviewer to verify that the expression on the right of the assignments matches my stated intention. Finally it removes the multiple return points, which can be controversial (although I'm sure some will defend them! ;))
 
Richard Innamorato
Greenhorn
Posts: 17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paul and Henry,

thanks for the explanation. Henry, your last advice obviously improved it a lot.... It cut lines of code down to only 2...

I just need to practice more and be more cognitive of where I can save and be less redundant.

Thanks to both of you.
 
Richard Innamorato
Greenhorn
Posts: 17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mike,

thanks for the advice. I just learned that I can use "||" in the return statement. I did not know that.


Thanks
 
Mike. J. Thompson
Bartender
Posts: 689
17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Richard Innamorato wrote:Mike,

thanks for the advice. I just learned that I can use "||" in the return statement. I did not know that.


Thanks


Absolutely, you can use any expression you like as long as it evaluates to the same type that the method returns.
 
Paul Clapham
Sheriff
Posts: 22846
43
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Richard Innamorato wrote:I just need to practice more and be more cognitive of where I can save and be less redundant.


That's not really where you should be focusing your efforts, though. As Mike pointed out, it's also important to make your code readable. And I would say that in a trivial piece of code like this, making it readable is far more important than considering whether it takes 85 or 97 nanoseconds to run.
 
Mike. J. Thompson
Bartender
Posts: 689
17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Richard Innamorato wrote:It cut lines of code down to only 2...


And just to add to what Paul said, reducing the number of lines of code is generally a good thing, but don't make it your ultimate aim. Afterall, you could write your entire program on one line of code if you wanted to but that would certainly not be a good refactoring.

The thing you should be striving for is simplicity and readability. These things will tend to produce fewer lines of code, but not at all cost.

There is also a little bit of subjectivity in there too. People may disagree sometimes about which code is simpler or more readable, but generally there will be a consensus.
 
Ranch Hand
Posts: 69
2
C++ Linux Netbeans IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
What's wrong with the built-in API for determining leap-years? Reinventing the wheel is one of many roots of sub-optimized code.

http://docs.oracle.com/javase/7/docs/api/java/util/GregorianCalendar.html#isLeapYear%28int%29

* Also, the code posted in the above replies subtly breaks with years BC.



Observing the above code, there seems to be an optimization strategy in place that provides a quick-return for 99.9 75% of input values. Naive implementations miss out on these "meta-branch-prediction" strategies.

* Markup on this site is a little funky. Should be finished editing now...
 
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Luke Leber wrote:Observing the above code, there seems to be an optimization strategy in place that provides a quick-return for 75% of input values.

And then a pile of unnecessary code for the rest of it.

<nitpick>
It also assumes that leap years existed in the Julian form before 45BC, which is not true, since Julian was itself a reform of the Roman calendar. It's a reasonable approach, but it ought to have been documented by a class that claims to encapsulate a Calendar.
</nitpick>

Winston
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!