• Post Reply Bookmark Topic Watch Topic
  • New Topic

can someone please criticize my code?  RSS feed

 
jon ninpoja
Ranch Hand
Posts: 291
3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
hi guys,

did a mini cattle market program to add cows to a list to dell,or remove etc
its pretty basic...

can you have a look at my code and tell me what i have wrong,what could be improved on etc etc
would really appreciate it

 
Jeanne Boyarsky
author & internet detective
Marshal
Posts: 37496
540
Eclipse IDE Java VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jon,
Fun. Cows! In fact, I'm going to give you a cow for practicing with something meaningful to the theme of this site!

You did many things well, but you asked us to "criticize" your code so focusing on the areas you can do something better:

  • while (choosing == true) is reudndant. You can just say while (choosing)
  • Have you learned about switch/case statements yet? It would make the if/else easier to read.
  • add_cows doesn't follow Java naming conventions. Remember that Java uses CambelCase by convention so it should be addCows
  • Usability: it would be good to tell the user the range expected rather than just "not available". Similarly, you could give a clearer error message if you separated the if statement
  • cows = cows + buyCow; could be cows += buyCow; Also buyCow sounds like an action. So maybe cows += numCowsToBuy;
  • '
  • Don't use \n at the end of a print() method call. Just call println() which takes care of the line break.
  •  
    Ganesh Patekar
    Bartender
    Posts: 726
    23
    Eclipse IDE Hibernate jQuery MySQL Database Spring Tomcat Server
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Good work Jon
    Few improvement IMO
  • Since user is not allowed to remove cow(s) when user has 0 cow here
  • then I think you also need to take care when user has bought 1 cow and then while removing cow enters remove 5 cows then It results -4. Prints You now have -4 cows. If later user think to buy 1 cow then It shows You now have -3 cows rather than You now have 1 cows.

  • You can declare int cows = 0 as private.
  • As Jeanne already mentioned about clear messages, you can show price of a cow to user at beginning. You can declare private final int cowCost = 500; and private int cowTotal = 0; in class where you declared int cows then you can calculate and show total amount of cows to user before checkout.
  • Also need to take care If user enters 'a' Or "eight" I mean invalid int value here
  •  
    jon ninpoja
    Ranch Hand
    Posts: 291
    3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    thanks for all the help amending the code...for my own reference

    are switches preferred to if statements?
    would you use them mostly? when does it matter whether you use switch or if?

    thanks again

     
    jon ninpoja
    Ranch Hand
    Posts: 291
    3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    is cows ok?
    as a variable name... or are you better using
    a more descriptive name like
    ranchCows etc

     
    Paul Clapham
    Sheriff
    Posts: 22831
    43
    Eclipse IDE Firefox Browser MySQL Database
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I'd say "cows" is a perfectly good variable name there. Now if your program allowed you to buy Ranch cows and Bart Simpson cows, you'd need longer variable names to keep them separate.
     
    jon ninpoja
    Ranch Hand
    Posts: 291
    3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    thanks paul what about switches and if's
    are switches mostly used? when would you choose if over a switch?
     
    Paul Clapham
    Sheriff
    Posts: 22831
    43
    Eclipse IDE Firefox Browser MySQL Database
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    When you have a list of possibilities, and you want to have different things happen for each of those possibilities, then a switch would be appropriate. For example your posted code from line 22 to 37, I would use a switch there rather than an if-else-else chain.

    There are other possibilities beyond that, for example you might have a Map<Integer, Action> where an Action is a class which contains code to do something specific. Then you'd take your input integer value, look in the Map to get the corresponding Action object, and use that to do whatever needs to be done. That's a design pattern called the Command pattern. But for a beginner who hasn't learned object-oriented design yet, that's too advanced. And anyway it would be a bit overkill for this program. Just be aware that large-scale Java applications look very different from what you're writing here, but you have to go through learning this sort of programming before you graduate to more real-world Java.
     
    jon ninpoja
    Ranch Hand
    Posts: 291
    3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    thanks paul,

    yes im feeling a little frustrated,im learning java on my own,and although i have made a lot of progress im still in the dark over a lot of issues...OOP being the main one
    should i even look at OOP or must i carry on doing what im doing? when will i know if im ready to start looking at OOP?

    im very interested in swing as well but cant seem to find the best way to make UI's in java...am i better off using the designer in netbeans...or learning from scratch

    do you work in the industry? it is a dream of mine to become a developer...even if it takes me another 3 to 5 years...at the moment im a general technician...hating every minute of it.
    all i want to do is code,but my job is getting in the way LOL!!!

    thanks again 
     
    Paul Clapham
    Sheriff
    Posts: 22831
    43
    Eclipse IDE Firefox Browser MySQL Database
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Well, let me ask you this: Are you following a formal course which is supposed to teach you Java, or are you just jumping into the pool and splashing around in the shallow end? It would be much better if you had a plan (imposed from outside via a teacher or a text or some other learning process).

    As for Swing: sure, you can use the Netbeans designer to throw together a basic UI. And I believe it has pretty good tutorials on how to do that. But what you end up with can easily be the equivalent of the giant main methods which you were writing. And it doesn't seem to take long (based on questions I've seen posted here) before you need to do something which is more than just wiring together a set of controls. That's the point where you need to understand that all of those things are objects and that they can talk to each other to get things done. And if your object-oriented design skills are zero (which unfortunately they are at the moment) then you're back in the pool, only this time at the deep end.

    So my recommendations: Get a program -- a book or a MOOC or something like that -- and work your way through it. I don't have any specific recommendations but there's this Java forum and if you ask there you're likely to get a lot of recommendations.
     
    Christopher Adams
    Greenhorn
    Posts: 17
    Chrome Java Windows
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I would say more // in code comments. I am also in the process of learning Java and I find it helpful
    to comment every step of the way. I think comments are very important in all levels of code
    but I think they are even more important when you are learning.
     
    Junilu Lacar
    Sheriff
    Posts: 11493
    180
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Some things to consider about comments:

    1. Prefer clear, expressive code over comments that explain WHAT the code is doing, or worse, HOW the code is doing something. If you have to write a comment to do that, then your code isn't clear and/or expressive enough.

    2. If there is ever a need for a comment, it should be so that other people who read the comments can understand the context in which a coding or design (often synonymous) decision was made. For example, why you decided to choose one algorithm over another. That is, the best comments are the ones that explain WHY the code was written or designed the way it was.

    3. Comments that look like they were written by Captain Obvious should obviously be deleted. For example, the following comments are silly and need to be removed.

     
    Bod MacNeil
    Ranch Hand
    Posts: 62
    2
    Java Mac Netbeans IDE
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I'm fairly new to program myself so I have a question for someone.  do we need to make an instance of the KeyChainsForReal class on lines 8 and 9 when we're already in that class?  Would it be better to just called the run() method on it own? 
     
    Ole Sandum
    Ranch Hand
    Posts: 76
    3
    IntelliJ IDE Java Windows
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Bod MacNeil wrote:Would it be better to just called the run() method on it own? 

    Did you try and see what happens if you do? If not, then I would urge you to do so. The result might surprise you
     
    Bod MacNeil
    Ranch Hand
    Posts: 62
    2
    Java Mac Netbeans IDE
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Ole Sandum wrote:
    Did you try and see what happens if you do? If not, then I would urge you to do so. The result might surprise you


    Its giving me an error because run() is not static.  I see now
     
    Campbell Ritchie
    Marshal
    Posts: 56546
    172
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Try calling run from inside an instance method and see whether that makes any difference. Beware: if you actually execute run like that, there is a risk of an infinite recursion, and you can see for yourself what problems that will cause
     
    Junilu Lacar
    Sheriff
    Posts: 11493
    180
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Bod MacNeil wrote:I'm fairly new to program myself so I have a question for someone.  do we need to make an instance of the KeyChainsForReal class on lines 8 and 9 when we're already in that class?  Would it be better to just called the run() method on it own? 

    These are actually fair and astute questions if you think about it.

    The creation of an instance of the class in this case is actually a silly hoop that Java makes you jump through because of the way Java was designed, not so much because of object-orientation in general. It's probably one thing that leads some experts to criticize Java as being a class-oriented language rather than a truly object-oriented one.

    In a program this small that solves a problem of very limited scope, there's no real benefit to object-orientation. This program could just as well have been written using all static methods with no significant impact to its correctness or quality.

    OO makes more sense and more of a difference when there are multiple ideas/abstractions about related logic and/or responsibilities that can be assigned to different "objects" in your code/design so that complexity is more manageable. There are many real benefits in introducing OO abstractions into your program design and taking advantage of OOP concepts like encapsulation, inheritance, and data hiding but only beyond a certain level of complexity where doing so is actually worth the extra effort needed.

    In this case, creating a single instance of the enclosing class is nothing more than a little pomp and ceremony that's required by Java protocol.
     
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!