• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Feedback on polymorphism exercise

 
Greenhorn
Posts: 24
1
Firefox Browser Linux Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi,
after learning about polymorphism, I tried to code a simple application that calculates how much an employer has to pay his employees.
Here is the code:



can someone please point out what I can do to improve my code? I would really appreciate it.
Kind regards,

Olivier
 
Bartender
Posts: 1357
39
IBM DB2 Netbeans IDE Spring Java
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Carefully have a look at your equals() methods. What may be wrong with them ?
 
Marshal
Posts: 79153
377
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Start by getting rid of the long lines, which make the code very difficult to read. Think how much worse it would be on paper. We suggest BSD/Allman indentation here, but if you are using K&R indentation, separate all { from what precedes them with a few spaces, and also divide }else{ with spaces.

I shall refer the post back to you; please make those edits on the original.
 
Olivier LeMaître
Greenhorn
Posts: 24
1
Firefox Browser Linux Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Claude Moore wrote:Carefully have a look at your equals() methods. What may be wrong with them ?


Honestly, I have no idea. firstly, I tried to compare the classes with the default equals() method, but the only output I got was "false", then I found this topic which said I had to overwrite the equals() method.

Campbell Ritchie wrote:Start by getting rid of the long lines, which make the code very difficult to read. Think how much worse it would be on paper. We suggest BSD/Allman indentation here, but if you are using K&R indentation, separate all { from what precedes them with a few spaces, and also divide }else{ with spaces.



Sorry, I've edited the original post.
 
Campbell Ritchie
Marshal
Posts: 79153
377
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Olivier LeMaître wrote:

Claude Moore wrote:Carefully have a look at your equals() methods. What may be wrong with them ?


Honestly, I have no idea. firstly, . . .

Campbell Ritchie wrote:Start by getting rid of the long lines . . .



Sorry, I've edited the original post.

No need to be sorry; that is one of the things you can expect if you ask for code to be reviewed. You have not quite managed it, I am afraid. Look at lines 6‑14 where you have incorrectly indented part and correctly indented a different part. Look at lines 191‑192, which are both too far to the right. You have changed to BSD indentation almost everywhere but left a little K&R indentation in line 199. You may think I am being too fussy about indentation, but it is an importatnt tool for you to see where you are in the code and it is you who will fail to indentify the errors because the indentation is not consistent.
You are also using the LF character with the \n escape. Only use \n if you have been told the LF character is required. Otherwise use printf and the %n tag which produces the correct line end for the operating system.

Your equals method is correct as it stands, but you have a much bigger problem there. Your understanding of what constitutes a class and an object is incorrect. All instances of the people class (which shou‍ld read Person, singular with CapitalLetter) have the same values for their fields. The different instances shou‍ld have different values for name and salary. You shou‍ld create constructors for your class and getXXX methods and toString. Only when you have different fields can you understand the equals method. You must also override the hashCode() method whenever you override equals. Start by constructing the classes representing different kinds of people, and you can look at equals and hashCode later.
 
Sheriff
Posts: 7125
184
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Some more minor suggestions:
  • The convention in Java is to name classes starting with a capital letter, so mainProgram would become MainProgram.
  • Get used to initializing a List like this: List<people> peopleList = new ArrayList<>();
  • You have a method named the same as the class: mainProgram().  This can easily be confused as a constructor.
  • In helperClass, you don't need two Scanners.
  • In helpClass#getUserInput, what happens if the user enters a letter?
  •  
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Your code can be improved drastically. There are both minor and major improvements which should be made to increase your code quality. Here's already a small list to start with:
  • please follow the naming conventions of the Oracle Code Conventions for the Java language (e.g. a class should be a noun with CamelCase, so starting with a capitalized letter)
  • you have a class people, I think it should be Person (or even better: Employee). This class has two instance variables: name and a salary. Both should be private (as you don't want other classes to change the values directly). And you should provide getters and setters, or have a constructor to set these values to create immutable objects. If you don't want instance of the Person class, make it abstract.
  • you have a bunch of other classes which extend from people and thus they already inherit the two instance variables and the getters. But for each and every class you define all these instance members again. Why?
  • why do you need two Scanner instances to get the user input?
  • in your overview() method you are iterating through an array using the enhanced for loop, but you are using a counter variable to keep track of the current index. So you should switch to a regular for loop instead
  • in your hirePeople() method you declare and increment a counter variable but don't use it


  • This gives you already a bit to work on, I guess. And finally you say that you created this exercise after reading about polymorphism, but actually very little polymorphism is happening in this exercise. Maybe you could give an employee a base salary of 1000, and then the salary of a cleaner would be 1.5 times the base salary, and for a guard it's 2.5x this salary. Then you would have polymorphism in action for the getSalary() method.

    ope it helps!
    Kind regards,
    Roel
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Thank you for the feedback everyone, I've tried to improve the code with your suggestions and repeated what objects and classes are.


    The things I've done:
  • changed println to printf
  • improved indentation
  • deleted the equals() method
  • deleted workman, cleaner and guard classes, so I am (hopefully?) using objects correctly now
  • changed List initialization (Not sure what the benefit is)
  • changed mainProgram() method to startMainProgram()
  • protected userInput in the helperClass
  • changed class people to Person
  • changed Person's instance variables to private
  • changed enhanced for loop in the overview() method to a normal for loop


  • Things I couldn't do:
  • I've tried to delete the second Scanner object in the HelperClass, but when I run the endProgram() method, it doesn't wait for input, and it cancels the exit
  • I do use the counter variable in the hirePeople() method

  •  
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:deleted the equals() method


    You have deleted the equals() method, but you are still invoking this method on line96. Do you have any clue why the program is still working as expected although you have deleted this method?

    Olivier LeMaître wrote:changed List initialization (Not sure what the benefit is)


    One of the OOP design principles is program to an interface (supertype), not an implementation. So in your code there is very little benefit (or even none), but it is a best practice to always program to an interface (supertype) and not an implementation.

    Olivier LeMaître wrote:I do use the counter variable in the hirePeople() method


    You are correct! I had missed it, but because you are using the counter variable, the same remark about enhanced for loop and regular for loop applies here as well

    A few minor remarks:
  • you still don't consistently follow the naming conventions as I spotted variable names starting with a capitalized letter
  • in the Person class you initialize the instance variable name with a default value, but you immediately overwrite this value in the constructor. So the default value is useless and prevents you from making the instance variable name immutable (marking it as a final variable)
  • there is a logical bug in the Person class: you have written additional code to prevent an invalid salary value. But what happens if I create a Person instance with a salary value of -100?


  • And now a more conceptual improvement. You have a list with possible persons you can hire and a list to store the hired persons. And you have lots of code to keep the lists in good shape when someone is hired or fired. You even create an additional array. This can be really simplified a lot. Assume you would add a property numberHired to the Person class. You could increment this property if a person is hired or decrement if a person is fired. No need to have hiredPeopleList anymore. And you could have a getTotalSalary() method, which simply multiplies numberHired with salary.

    Hope it helps!
    Kind regards,
    Roel
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I am trying to improve the code, following your suggestions, Roel.
    I just have one (probably stupid) question: I try to exit the program when an invalid salary is given, so I add in the Person constructor. Why does this give me an "cannot find symbol" error?
    Thanks a lot

    ps: I live in Antwerp too
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:Why does this give me an "cannot find symbol" error?


    Probably because program is not defined/known in the Person constructor. But even if it didn't give a compiler error, this would not be the desired solution. You should throw a (checked) exception when salary is invalid and then you can appropriately handle this exception in your main program. In fact, these values are not entered by the user, but hard-coded by the developer. So probably the best choice would be a runtime IllegalArgumentException which is not caught nor handled as it indicates a mistake by the developer not by the user. So if this scenario occurs, the program should not continue anymore as it can result in unpredicted and undesired behavior.
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    You are correct! I had missed it, but because you are using the counter variable, the same remark about enhanced for loop and regular for loop applies here as well  


    I've improved that.

    in the Person class you initialize the instance variable name with a default value, but you immediately overwrite this value in the constructor. So the default value is useless and prevents you from making the instance variable name immutable (marking it as a final variable)


    I set it before there was a constructor, so I could easily debug it. But I have removed it now

    And now a more conceptual improvement. You have a list with possible persons you can hire and a list to store the hired persons. And you have lots of code to keep the lists in good shape when someone is hired or fired. You even create an additional array. This can be really simplified a lot. Assume you would add a property numberHired to the Person class. You could increment this property if a person is hired or decrement if a person is fired. No need to have hiredPeopleList anymore. And you could have a getTotalSalary() method, which simply multiplies numberHired with salary.


    Yes, thank you, a lot simpler indeed.

    You have deleted the equals() method, but you are still invoking this method on line96. Do you have any clue why the program is still working as expected although you have deleted this method?


    I have no idea. Maybe I was too optimistic, thinking I could make this application (and understand how it works), It will probably be better if follow the book more strictly. thanks everyone for the help :)
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:I have no idea.


    Let's shed some light on this by giving nothing but a few pointers and let you discover the reason why :)

    You deleted the equals() method, but the line invoking this method still compiled successfully. Do you know why you didn't get a compiler error on this invocation?
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Roel De Nijs wrote:

    Olivier LeMaître wrote:I have no idea.


    Let's shed some light on this by giving nothing but a few pointers and let you discover the reason why :)

    You deleted the equals() method, but the line invoking this method still compiled successfully. Do you know why you didn't get a compiler error on this invocation?



    I know that equals() compares two objects with each other instead of "==" which only compares the reference variables. Maybe it works because it are instances of the same class, and when all the instance variables(is that the same as the members?) of two objects are equal it returns true, otherwise false?
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Roel De Nijs wrote:
    You deleted the equals() method, but the line invoking this method still compiled successfully. Do you know why you didn't get a compiler error on this invocation?



    Because it is part of the java.lang package and we don't have to import it?
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:Because it is part of the java.lang package and we don't have to import it?


    Definitely not! This makes even no sense. equals() is an instance method, so that's completely unrelated with packages. Classes and interfaces can be used in an import statement (and static members as well with a static import), but instance members can't be used in an import statement.
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:I know that equals() compares two objects with each other instead of "==" which only compares the reference variables.


    That's spot-on! But the question remains: you have deleted the equals() method, so why does that statement still compile? As you have deleted the equals() method, you might expect a "can't find method" compiler error, but instead the code compiles successfully. Can you explain why? Or do you have no clue?
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Roel De Nijs wrote:But the question remains: you have deleted the equals() method, so why does that statement still compile? As you have deleted the equals() method, you might expect a "can't find method" compiler error, but instead the code compiles successfully. Can you explain why? Or do you have no clue?



    Stackoverfow sais it is provided by "java.lang.Object", does that mean it is part of a set of default methods that every object gets? Or am I just talking gibberish?
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:Stackoverfow sais it is provided by "java.lang.Object", does that mean it is part of a set of default methods that every object gets? Or am I just talking gibberish?


    StackOverflow is correct! I wonder which book you are currently reading. Because it seems you didn't know the java.lang.Object class and that's the mother of all classes! So it is a bit strange that you would be thaught about polymorphism, but not yet about the most fundamental class of the Java API

    You write the following codebut that's short-hand syntax forSo if a class doesn't explicitly extend from another class, it will extend from java.lang.Object. That means that every class will always extend from java.lang.Object (either directly or indirectly). And when you extend from a class, all visible methods defined in the superclass will be inherited by the subclass. And the equals() method is one of those methods. Another famous method is the toString() method. And the implementation of the equals() method in the java.lang.Object class looks like thisSo that's why this method returns true if and only if both reference variables refer to the same object.
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    The equals method is just the == operator brought down to a lower level? But why didn't the not overwritten equals() method return false, when I compared two identical objects in my original code?
     
    Saloon Keeper
    Posts: 10687
    85
    Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows ChatGPT
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Roel De Nijs wrote:And now a more conceptual improvement. You have a list with possible persons you can hire and a list to store the hired persons. And you have lots of code to keep the lists in good shape when someone is hired or fired. You even create an additional array. This can be really simplified a lot. Assume you would add a property numberHired to the Person class. You could increment this property if a person is hired or decrement if a person is fired. No need to have hiredPeopleList anymore. And you could have a getTotalSalary() method, which simply multiplies numberHired with salary.


    I think this is important. One problem I have is that you have a "Person" class. Based on your usage this seems to be more of a "JobClass" class, or just "Job". A "workman" is a job classification, "Joe Smith" is a person. If your Job/Person class maintains a count then there's no need for some of the array manipulation you're doing and a getTotalSalary() method would be a piece of cake. It would also fix a problem I see with your fire() method which is that currently fire() will remove an entire job class instead of letting one person go of that job class.
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:The equals method is just the == operator brought down to a lower level?


    Not really, it is nothing more but the implementation of the equals() method in the Object class. And the Java language designers dediced back in the days to return true if both reference variables refer to the same object. Maybe on another day they would have decided to simply return false as the implementation (meaning two objects will never be equal). But if you think about it, the current implementation makes sense: if both reference variables refer to the same object, both objects must be equal.

    Olivier LeMaître wrote:But why didn't the not overwritten equals() method return false, when I compared two identical objects in my original code?


    I don't completely understand this question. Can you try to rephrase and/or explain it a bit better?
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Carey Brown wrote:One problem I have is that you have a "Person" class.


    You are spot-on! With the suggested improvement of adding a numberHired property, the class name should be changed as well as Person is not appropriate anymore.
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Does equals also return true when two objects are instances of the same class but have different instance variables? (no I guess?)

    I don't completely understand this question. Can you try to rephrase and/or explain it a bit better?



    when I ran my original code (top of the post) without overwriting the equals() method for the object, equals() (like used on line 91) always returned false, even if it was the same object.

    I think this is important. One problem I have is that you have a "Person" class. Based on your usage this seems to be more of a "JobClass" class, or just "Job"



    You are all right, I was not thoughtful enough with my naming sceme
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:Does equals also return true when two objects are instances of the same class but have different instance variables? (no I guess?)


    The equals() method of class Object will only return true if both reference variables refer to the same object; it will return false otherwise. Because both reference variables refer to the same object, you can't have different instance variables as you only have one object. If you are looking for two objects being meaningfully equivalent (e.g. two Person objects/instances are considered to be equal if both have the same social security number) you will need to override the equals() and add your own implementation.

    Olivier LeMaître wrote:when I ran my original code (top of the post) without overwriting the equals() method for the object, equals() (like used on line 91) always returned false, even if it was the same object.


    That's really very hard to believe Because that would mean there is a very crucial bug in the JDK. If you are using equals() method of class Object, true is returned if both reference variables refer to the same object.
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I think I finally understand!
    I wanted to ask you why the equals method does  work on line 96, because I thought the variables stored in hiredPeopleList aren't refering to the same objects as those in peopleList.
    Then I looked at line 79, there isn't a new object being instanciated, it is the same object, just an other reference variable in an other ArrayList
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:I wanted to ask you why the equals method does  work on line 96, because I thought the variables stored in hiredPeopleList aren't refering to the same objects as those in peopleList.
    Then I looked at line 79, there isn't a new object being instanciated, it is the same object, just an other reference variable in an other ArrayList


    Exactly! You are spot-on!
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Roel De Nijs wrote:
    Exactly! You are spot-on!


    Yes! Thank you for your patience and for your time! :)
    I think I am going to make a heavily improved second version, do you think it is a good idea if I post it here? Or is it better to just keep it just for my self? (I don't want to waste anybody's time even more than I did now :)  )
     
    Roel De Nijs
    Sheriff
    Posts: 11604
    178
    Hibernate jQuery Eclipse IDE Spring MySQL Database AngularJS Tomcat Server Chrome Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Olivier LeMaître wrote:do you think it is a good idea if I post it here? Or is it better to just keep it just for my self?


    You will only be able to improve your coding skills if you get some feedback from other (more experienced) developers. If the code is somewhere on your computer, you'll miss a great opportunity to improve your Java skills/knowledge and becoming a better Java developer.
     
    Knute Snortum
    Sheriff
    Posts: 7125
    184
    Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    I think I am going to make a heavily improved second version, do you think it is a good idea if I post it here?


    Absolutely!  This is why we're all here.
     
    Sheriff
    Posts: 17644
    300
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Likes 2
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    1. Keep scope as small as possible.

    isRunning has default (package private) scope. Does its scope need to be that wide? How small can you bring its scope down? Can you think of a way to make it local? Why can't you make it local? (The answers to some of these questions will point to code smells -- things that are not quite right in your code/design)

    2. Try different names to see how they would change the semantics of the code. Very often, the first name we use is not the best name that will clearly express our intent. In fact, I find that most of the time, the first name you choose is not at all the one that should be used. That's because you can almost always find a better way of expressing the intent of the code.

    Here's what goes through my mind when I read that part of the code:

    Where is isRunning declared? (scan upwards for a declaration). Oh, it's at the instance level... Oh, it's got package private scope... Why package private? Where is it getting set?! (scan downward) Oh, it's probably in this endProgram() method... (scan downward some more) Yeah, there it is. Huh.  So, if endProgram() is true, then we don't actually end the program. If it's false, then we end the program. Wait, what?! Oh, endProgram() actually doesn't return a boolean, it's void. So when we say endProgram() it may or may not actually end the program.

    Do you see how your choice of names makes your program express ideas in a confusing way?

    One strategy I take in trying to find better names is to do a 180-degree flip in perspective. What if we use a name that's more consistent with the idea of endProgram() returning true if the user confirms he/she wants to quit? After all, that's the idea that the name "endProgram" conveys, isn't it? Or maybe we want a different way to express "end program"? Maybe this would be a better way to say it:

    Note that two names were changed: isRunning to displayMenu and endProgram to cancelExitRequest. Read that code and analyze the semantics: If the user cancels his request to exit the program (returning true), then display the menu again. Otherwise, don't display them menu. Doesn't that make more sense now?

    Note: the road from "isRunning/endProgram" to a better choice like "displayMenu/cancelExitRequest" may not be as straightforward and easy as it appears to be above. You'll probably try a few other things that don't work either. You could either keep trying until you find something that makes sense or cop out and say "Well, they'll know what I mean" and go back to what you originally had. The right way is not alway the easiest way and unfortunately, from my experience, most programmers choose the easy way and cop out. Hopefully, you'll learn to persevere and choose to do the right thing even if it takes a little more effort.
     
    Junilu Lacar
    Sheriff
    Posts: 17644
    300
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Roel De Nijs wrote:

    Olivier LeMaître wrote:changed List initialization (Not sure what the benefit is)


    One of the OOP design principles is program to an interface (supertype), not an implementation. So in your code there is very little benefit (or even none), but it is a best practice to always program to an interface (supertype) and not an implementation.


    More generally, you should program to abstractions, not concretions. The underlying principle is LSP or Liskov's Substitution Principle
     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I have made a (hopefully) better version,
    here is the code:



    The things I've done different:
  • Avoided long lines and used '%n' instead of "\n"
  • Made sure I used the naming convention
  • used only one Scanner object in the Helper class
  • I used an instance variable 'amountOfStaff' in the Staff class


  • Thank you all for the help, I hope I've improved with this version :)
     
    Knute Snortum
    Sheriff
    Posts: 7125
    184
    Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    It looks really good!  I've had to get very picky to find anything.
  • When you continue a statement on a second line, you should indent two tabs like so:


  • Really, that's the only suggestion.

    If you wanted to go farther with this, how about separating the classes into different files, setting a package, setting class and method access modifiers, documenting public methods and classes with Javadocs.
     
    Carey Brown
    Saloon Keeper
    Posts: 10687
    85
    Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows ChatGPT
     
    Ranch Hand
    Posts: 234
    12
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I noticed that the constructor in class Staff calls a non-final method. The Java™ Tutorials warn against this because during initialization of a subclass instance, the constructor may indirectly access instance fields in the subclass instance before they have been initialized.

    Java™ Tutorials (Initializing Fields) says:

    ...calling non-final methods during instance initialization can cause problems.


    Effective Java™ Second Edition (page 89) says:

    Constructors must not invoke overridable methods, directly or indirectly.

     
    Olivier LeMaître
    Greenhorn
    Posts: 24
    1
    Firefox Browser Linux Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Knute Snortum wrote:It looks really good!  I've had to get very picky to find anything.

  • When you continue a statement on a second line, you should indent two tabs like so

  • If you wanted to go farther with this, how about separating the classes into different files, setting a package, setting class and method access modifiers, documenting public methods and classes with Javadocs.



    Thank you for reviewing the code! I think I am going to enhance my theoretical knowledge of Java,before trying to do anything concrete again

    Daniel Cox wrote:I noticed that the constructor in class Staff calls a non-final method. The Java™ Tutorials warn against this because during initialization of a subclass instance, the constructor may indirectly access instance fields in the subclass instance before they have been initialized.



    Thank you, I have fixed it.
    reply
      Bookmark Topic Watch Topic
    • New Topic