• 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

Java code improvement.

 
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hello everyone.

I'm learning Java (albeit, in University), and was tasked with writing some program which takes a file, and utilizes StringTokenizer, cleans it up and prints things back to the user, i.e., name, age, et al.

So, the second bit of the task is to make routines maxAge(), minAge(), and avgAge(), all of which I've done shown here:

It's quite a bit of code, separated by different source files. The one in question is Person.java


It's sloppy, and can certainly be done better, but I'm not sure how. Each class has methods, and the Person.java class has methods that are integrated into the constructors, i.e., Person.getAge(), and the maxAge() methods are done using static variables, I feel it can be done better and in a new class, accepting a List <Person> as an argument, but even that seems a bit sketchy.

The next bit of the task is to make the routines faster, but I think I've done it all so badly that I may need to rewrite.

Could anyone point me in the right direction?

(Literally been programming in Java for less than a month.)


 
Marshal
Posts: 28175
95
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I'm sorry, but I don't even understand what the methods you wrote are supposed to be for.

For example "maxAge" (which it would be an improvement to call it "maximumAge")... what's the maximum age of a Person? I would have thought a Person only has one age. But your code doesn't even support the idea that a Person has an age, because the "age" variable which you set in the Person constructor is a static variable. This means that "age" is the same for all Person instances.
 
Sheriff
Posts: 67746
173
Mac Mac OS X IntelliJ IDE jQuery TypeScript Java iOS
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Aside from all the statics (what is up with that?), here are a few off-the-cuff things I'd advise:

  • Get out of the habit of freeze-drying variables names. ln is bad; lastName is good. fn is bad; firstName is good. And a is just horrible.


  • Why is the age being passed in as a String that needs to be parsed? Declare the parameter as the type it should be.


  • What's the access type of your non-static instance variables? They should be private unless you have a really good reason otherwise.


  • Avoid superfluous comments. Just like the boy who cried wolf, superfluous comments cheapen the value of useful comments. "Make all last names uppercase" is not necessary -- it's quite clear form the code what's going on. Save the comments for things that really need it.


  • Don't put return values in parentheses unnecessarily. It just adds noise to the code.
  •  
    Dustin Van der Woodsen
    Greenhorn
    Posts: 3
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Bear Bibeault wrote:Aside from all the statics (what is up with that?), here are a few off-the-cuff things I'd advise:

  • Get out of the habit of freeze-drying variables names. ln is bad; lastName is good. fn is bad; firstName is good. And a is just horrible.


  • Why is the age being passed in as a String that needs to be parsed? Declare the parameter as the type it should be.


  • What's the access type of your non-static instance variables? They should be private unless you have a really good reason otherwise.


  • Avoid superfluous comments. Just like the boy who cried wolf, superfluous comments cheapen the value of useful comments. "Make all last names uppercase" is not necessary -- it's quite clear form the code what's going on. Save the comments for things that really need it.


  • Don't put return values in parentheses unnecessarily. It just adds noise to the code.


  • It's really part of a larger set of classes. The statics are there because they get the ages, and so on from a line, from a file, which is parsed, as a List, split into separate parts (age, name, etc), and then stored.
    So that's the only feasible way I saw to do this.

    Age is hence being passed in as a String because it's all part of a bigger string, gotten from a FileReader.

    Regarding the comments, yeah, you're right. Would you like a look at the entire thing?
     
    Java Cowboy
    Posts: 16084
    88
    Android Scala IntelliJ IDE Spring Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    When you write an object oriented program and you are thinking about what classes to add, you often make classes that represent something in the real world, and you'd add properties (member variables) to the class that make sense.

    Let's look at your class Person. Ok, it has member variables firstname and lastname (lines 8 and 9), that makes sense.

    But what about all those static member variables (lines 4, 5, 6)? They don't really make sense for a class Person. Note that static means that there's only one copy of the variables that's shared between all instances of class Person (they are class variables - see Understanding Instance and Class Members for more information).

    It doesn't make sense that there is an age variable which is shared between all persons. What's worse is that in the constructor, in line 19, you're setting the static age variable - that means that age will hold the value for the last Person object that was created.

    Note that in the toString method, in line 48, you're printing the age as if it's the age of the particular Person object that you call toString on. But it's not! If you make a number of Person objects with different ages and then print them out with toString, you'll see that all of them have the same age - that's most likely not what you meant.

    The private no-args constructor in line 11 is unnecessary. Note that the Java compiler automatically adds a public no-args constructor to your class, but only if the class doesn't have any constructors defined. Since you already have a constructor (lines 13-20), the constructor in line 11 is unnecessary.

    In my opinion, you should put the logic of determining the youngest and oldest person in a separate class, that works on a list of persons and finds the oldest and the youngest.

    Dustin Van der Woodsen wrote:Age is hence being passed in as a String because it's all part of a bigger string, gotten from a FileReader.


    You should parse it into an int at the place where you're reading it from the file, and pass the int to class Person. Class Person shouldn't have to know where the value comes from, so it shouldn't have to have code to deal with the particular way it's read out of the file.
     
    Marshal
    Posts: 79151
    377
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Dustin Van der Woodsen wrote: . . . some program which . . . and utilizes StringTokenizer, . . .

    Have you really been told to use a tokenizer? If you look here, it tells you not to use a tokenizer in new code. It also make a suggestion of what to use instead. I think you might have to query that with your teachers.
    An alternative is to use a Scanner; there are more details in the Java Tutorials.

    And welcome to the Ranch
     
    Dustin Van der Woodsen
    Greenhorn
    Posts: 3
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Campbell Ritchie wrote:

    Dustin Van der Woodsen wrote: . . . some program which . . . and utilizes StringTokenizer, . . .

    Have you really been told to use a tokenizer? If you look here, it tells you not to use a tokenizer in new code. It also make a suggestion of what to use instead. I think you might have to query that with your teachers.
    An alternative is to use a Scanner; there are more details in the Java Tutorials.

    And welcome to the Ranch



    Thanks a lot, for the words of welcome.

    Yes, I've been instructed to use StringTokenizer, I know it's inefficient, and there are better ways to separate tokens, but it's just the second week, and this is just to illustrate how tokens work I suppose.

    The program itself, its efficiency in Person.java, shown above is poor. And also relates to the rest of the program.

    I'm not sure how I would, for example, go about doing this:



    To

     
    Campbell Ritchie
    Marshal
    Posts: 79151
    377
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    You should never use == true or similar. They are not only poor style, but also error‑prone if you write = instead of == by mistake. It is while (check)... not while (check == true)...
    You should make all your fields private (unless they are global constants).
    A method like averageAge() does not belong in the Person class because one Person object does not “know” any other Person object’s age. You can put the average age method in a different class, however, maybe wherever your List lives. [Edit]That is similar to what Jesper said earlier about youngest and oldest.[/edit]
     
    Bartender
    Posts: 3323
    86
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Campbell Ritchie wrote:A method like averageAge() does not belong in the Person class because one Person object does not “know” any other Person object’s age. You can put the average age method in a different class, however, maybe wherever your List lives.


    Isn't this what the OP has done already by moving these methods to a People class which contains a List of type Person.

    @Dustin Van der Woodsen
    The implementation of the People class is confused. If you want to pre-calculate values such as max, min, first, sum and count then you need to do it in the constructor and you need to make sure the Person class and the People classes List are immutable. If they aren't immutable then it will be difficult to impossible to keep the pre-calculated values in sync with the list's contents. The other option is to not pre-calcuate any of these values and in each method iterate over the list of Person objects and calculate the appropriate value for that method.
     
    Campbell Ritchie
    Marshal
    Posts: 79151
    377
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Tony Docherty wrote: . . . Isn't this what the OP has done already . . .

    Yes. I realised Jesper had made the suggestion, but didn’t realise OP had already carried it out.
     
    Bartender
    Posts: 10780
    71
    Hibernate Eclipse IDE Ubuntu
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Dustin Van der Woodsen wrote:I'm not sure how I would, for example, go about doing this:


    Well, adding a People class is certainly a step in the right direction, because a single Person can't have a 'maximum' (or 'average') age, but a set of People can.

    And therein lies another problem: Why have you made People a List? Lists allow duplicates, and I suspect that you don't want duplicate Persons in your People object. My advice: Have a look at java.util.Set instead.

    It may also be worth mentioning that most List and Set classes are not final. How do you think that might help you?

    Winston
     
    Consider Paul's rocket mass heater.
    reply
      Bookmark Topic Watch Topic
    • New Topic