File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
The moose likes Beginning Java and the fly likes Java code improvement. Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "Java code improvement. " Watch "Java code improvement. " New topic
Author

Java code improvement.

Dustin Van der Woodsen
Greenhorn

Joined: Jan 18, 2013
Posts: 3
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.)


Paul Clapham
Bartender

Joined: Oct 14, 2005
Posts: 18882
    
    8

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.
Bear Bibeault
Author and ninkuma
Marshal

Joined: Jan 10, 2002
Posts: 61606
    
  67

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.


  • [Asking smart questions] [Bear's FrontMan] [About Bear] [Books by Bear]
    Dustin Van der Woodsen
    Greenhorn

    Joined: Jan 18, 2013
    Posts: 3
    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?
    Jesper de Jong
    Java Cowboy
    Saloon Keeper

    Joined: Aug 16, 2005
    Posts: 14338
        
      22

    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.

    Java Beginners FAQ - JavaRanch SCJP FAQ - The Java Tutorial - Java SE 8 API documentation
    Campbell Ritchie
    Sheriff

    Joined: Oct 13, 2005
    Posts: 39784
        
      28
    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

    Joined: Jan 18, 2013
    Posts: 3
    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
    Sheriff

    Joined: Oct 13, 2005
    Posts: 39784
        
      28
    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]
    Tony Docherty
    Bartender

    Joined: Aug 07, 2007
    Posts: 2364
        
      50
    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
    Sheriff

    Joined: Oct 13, 2005
    Posts: 39784
        
      28
    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.
    Winston Gutkowski
    Bartender

    Joined: Mar 17, 2011
    Posts: 8192
        
      23

    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

    Isn't it funny how there's always time and money enough to do it WRONG?
    Articles by Winston can be found here
     
    I agree. Here's the link: http://aspose.com/file-tools
     
    subject: Java code improvement.