Win a copy of Mesos in Action this week in the Cloud/Virtualizaton forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

Website for code reviews?

 
Geo Gavin
Greenhorn
Posts: 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I am just starting to write Java programs. I wanted to know if there are websites where people reviews each others code and comment about do's and donts ... Books may help but I believe to master a skill you need to practice and get hands dirty. Help will be appreciated.

Thanks in advance.

I wrote a Money Dispenser program

I wanted to know if my approach is correct.

My idea was to write a code that will generate change dispensed for a given amount of money.
ChangeComputer.java - This is where logic resides which decides how much money in what unit is dispensed

Input $11.53
Output 1 - $10 bill, 1 - $1 bill, 2 - Quarters, 3 - pennies





-------------
DispenseAmount.java is the class that describes the dispensed amount (money in different form of units (penny, quarter, dollar bill, $100 bill etc )).




This is the class where main resides and accepts an input argument for amount we want to dispense.
-----------------



 
Andy Jack
Ranch Hand
Posts: 257
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I wish I was smart enough to understand your code the moment I saw it. Please describe your code in detail with comments. Nobody has the time to go through lots of code.
To see what it feels like if you were in our position, please describe the code below and tell me if there is a problem in it.



Makes sense ?
 
Joanne Neal
Rancher
Posts: 3742
16
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Geo Gaga wrote:I wanted to know if my approach is correct.

The first thing you need to do is to read UseCodeTags.
Andy has done this and you can see how much easier it is to read his code. People are more likely to help you if you make it easier for them.
 
Winston Gutkowski
Bartender
Pie
Posts: 10417
63
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Andy Jack wrote:please describe the code below and tell me if there is a problem in it.

Andy,

Please DontWriteLongLines. I've broken yours up this time (line 57).

Thanks

Winston
 
Nam Ha Minh
Ranch Hand
Posts: 502
Eclipse IDE Firefox Browser Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have seen a code review website (sorry I don't remember its name). I think there are such sites out there. And I agree with other buddies, the most important in code review is making it easy to review. Don't post bulky code.
 
fred rosenberger
lowercase baba
Bartender
Posts: 12124
30
Chrome Java Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The first and only thing anyone should say about this code is that there are basically no comments.

There is no reason to look at anything else until that is fixed.

Just my 2-cents.
 
Geo Gavin
Greenhorn
Posts: 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks all for responses. I added comments and code is now formatted. Thanks for the suggestions.
 
Campbell Ritchie
Sheriff
Pie
Posts: 48954
60
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Now it’s formatted, let’s have a look at it.

And you realise you have already found the best place for code reviews?
 
Tina Smith
Ranch Hand
Posts: 208
9
Eclipse IDE Firefox Browser Java
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have a couple questions about the datatypes you've used in your code.

1. What is the reason behind choosing BigDecimal as the object to do the calculations from in ChangeComputer?
2. Why do you declare constants HUNDREDVALUE (etc) as a BigDecimal than use it as an Integer?

Points:
I think HUNDREDVALUE should be HUNDRED_VALUE to split the words out so it's readable. Not so bad in this case but imagine if a word could be interpreted two different ways with and without the space.
You are using int in DispenseAmount but BigDecimal in ChangeComputer - this is inconsistent.

On comments....
I don't see any comments in the code at the beginning of this post. Maybe what you meant is that you put them in your local copy? If so, can you either update or post your changes(I kind of prefer updating since it means less to scroll through but could also be confusing since a lot of the posts that follow are about no-comments) so that we can see what you see?
At work, I put a comment above every method to describe what that method does. I don't think that's necessarily required for getter/setter methods but it should be done for methods that do any logic. Then I can skim code and see at-a-glance what this method is supposed to do, rather than figuring it out every time. Picture yourself six months in the future. If you needed to tell someone what this code does and you had only 3 minutes to look at the code before describing it to them, would you be able to do it? Would you be able to make changes to the code easily? Use these as a guideline for how many comments to put in. For a beginning programmer, that's usually a lot of comments (at least it was for me).
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic