11
\$\begingroup\$

Doing practise questions for a Java exam which have no answers (useful) I have been asked to do the following:

Write a class called Person with the following features:

  • a private int called age;
  • a private String called name;
  • a constructor with a String argument, which initialises name to the received value and age to 0;
  • a public method toString(boolean full). If full is true, then the method returns a string consisting of the person’s name followed by their age in brackets; if full is false, the method returns a string consisting of just the name. For example, a returned string could be Sue (21) or Sue;
  • a public method incrementAge(), which increments age and returns the new age;
  • a public method canVote() which returns true if age is 18 or more and false otherwise.

My code is as follows can someone point out any errors?

public class Person
{
    private int age;
    private String name;

    public Person(String st)
    {
      name = st;
      age = 0;
    }

   public String toString(boolean full)
   {
     if(full == true)
     {
        return name + "(" + age + ")";
     }
     else
     {
        return name;
     }
   }

   public int incrementAge()
   {
      age++;
      return age;
   }

   public boolean canVote()
   {
     if(age >= 18)
     {
        return true;
     }
     else
     {
        return false;
     }
   }
}
\$\endgroup\$

migrated from stackoverflow.com Sep 23 '11 at 23:06

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ I don't see any errors :) \$\endgroup\$ – Cata May 30 '11 at 14:27

15 Answers 15

14
\$\begingroup\$
 return name + "(" + age + ")";

to

return name + " (" + age + ")";
\$\endgroup\$
14
\$\begingroup\$

instead of

if(full == true)

use

if (full)

and instead of

if(age >= 18)
{
   return true;
}
else
{
   return false;
}

use:

return age >= 18
\$\endgroup\$
  • \$\begingroup\$ Thank you both for your answers, just wee small minor things i need to keep an eye on then. \$\endgroup\$ – user445714 May 30 '11 at 14:31
11
\$\begingroup\$

That's fine, but I would have written the last method as return (age >= 18);

\$\endgroup\$
9
\$\begingroup\$

I don't think it's a good idea to write toString method with arguments - doing so you will not override the default Object.toString and will have no benefits from using it outside your code. For example, when outputting elements of a collection which contain Person won't display neither age nor name, but something like Person@a3f33f (default implementation). At least provide toString with no arguments as well

\$\endgroup\$
  • \$\begingroup\$ Denisk that actually moves onto my next question, Does the Person class overload or override any methods? Briefly explain your answer. \$\endgroup\$ – user445714 May 30 '11 at 14:34
  • \$\begingroup\$ I was going with override until i seen your comment and would go with overload the Object.toString? \$\endgroup\$ – user445714 May 30 '11 at 14:35
  • \$\begingroup\$ Currently you have toString method overloaded - thus, you actually have 2 toString methods - one inherited from Object (default no-arg method), and one with boolean argument. If you declared your toString with no arguments it would override Object.toString, and you would end up with 1 toString method (which, as I have mentioned, can be widely used by many things outside your own code) \$\endgroup\$ – denisk May 30 '11 at 14:38
  • \$\begingroup\$ I agree with denisk, and want to point out that flags like full in method arguments are a code smell: A method should do one and only one thing (Single Responsibility Principle), and a method with a flag does at least two things. Uncle Bob agrees as well (Clean Code: amazon.de/Clean-Code-Handbook-Software-Craftsmanship/dp/… ) \$\endgroup\$ – Landei Mar 3 '12 at 13:44
  • \$\begingroup\$ The toString(boolean) method is part of the assignment requirements. But normally, this is good advice. \$\endgroup\$ – Eva Jan 16 '13 at 12:04
5
\$\begingroup\$

In addition to the other comments, on your constructor, having an argument called "st" isn't particularly useful to the reader as it doesn't tell them what it's for. From a code style perspective might be better to have an argument called name, and explicitly set this.name = name in the body.

\$\endgroup\$
  • 1
    \$\begingroup\$ To add on, at this moment of learning it may seem not to matter, but when you're developing, these little things that make the usage more intuitive (especially when you're using auto-completion) will reduce your (and others') pain. \$\endgroup\$ – blizpasta May 30 '11 at 14:44
5
\$\begingroup\$

Your code is almost fine. I would change two things:

The following is a little shorter :)

public boolean canVote()
{
    return age >= 18;
}

Your toString(true) would return "Sue(21)" rather than the "Sue (21)" (with space) which is asked for. My toString would look like this:

public String toString(boolean full)
{
    String str = name;
    if(full) {
       str += " (" + age + ")";
    }

    return str;     
 }
\$\endgroup\$
4
\$\begingroup\$

If the age is implemented as a primitive integer there is no need to initialize it to 0 because all primitive number instance variables are initialized to 0 by default. Anyway if you had used a wrapper instead (Integer) the initialization to 0 is required.

Take a look at the "Default Values" section:

http://download.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html

\$\endgroup\$
3
\$\begingroup\$

How about adding a constant for 18, say MIN_AGE_TO_VOTE?

And:

return name + (full ? " (" + age + ")" : "");

Less readable, I agree :-)

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for only the constant. (I agree, the ternary operator is not too readable and harder to maintain.) \$\endgroup\$ – palacsint Jul 7 '12 at 15:56
2
\$\begingroup\$

Looks fine, only canVote is maybe a bit clumsy:

   public boolean canVote()
   {
        return age >= 18;
   }
\$\endgroup\$
2
\$\begingroup\$

I would see if you can make the code less verbose.

public boolean canVote() {
    return age >= 18;
}
\$\endgroup\$
2
\$\begingroup\$

This is also less verbose

public int incrementAge() {
  return ++age;
}
\$\endgroup\$
  • \$\begingroup\$ I worry that a student might be docked points for readability if they put that answer. Otherwise, I would write it like that too. \$\endgroup\$ – Eva Jan 16 '13 at 12:07
2
\$\begingroup\$

Use String.format instead of string concatenation:
return name + "(" + age + ")"; becomes

return String.format("%s (%s)", name, age);
\$\endgroup\$
  • \$\begingroup\$ It's a Java question ;) \$\endgroup\$ – palacsint Jul 7 '12 at 22:09
2
\$\begingroup\$
  1. The name field could be final. Making a variable final relieves the programmer of excess mental juggling - he/she doesn't have to scan through the code to see if the variable has changed. (From @nerdytenor's answers.)

  2. You should check the argument in the constructor. Does it make sense to create a Person object with a name which is null or an empty String? If not, check it and throw a NullPointerException or an IllegalArgumentException. (Effective Java, Second Edition, Item 38: Check parameters for validity)

  3. A note about the specification:

    Flag arguments are ugly. Passing a boolean into a function is a truly terrible practice. It immediately complicates the signature of the method, loudly proclaiming that this function does more than one thing. It does one thing if the flag is true and another if the flag is false!

    Source: Clean Code by Robert C. Martin, Chapter 3: Functions.

    This should be two methods: toString() and toFullString(), for example, without any parameter.

\$\endgroup\$
1
\$\begingroup\$
public Person(String st)
{
  name = st;
  age = 0;
}

st is not a good name for a variable or parameter, because it does not tell you what it contains. It implies that it means "string", but even that is not clear. Call it name instead. Use this.name = name to distinguish member and parameter names.


Put the opening brackets on the first line of the block, not at the beginning of the next line, since it is common practise in Java (even though I find it prettier if it is on the next line, since I code C# more than Java). Other Java developers reading your code will be used to it.


Others have already pointed it out: Write if (full) instead of if (full == true). That is clearer and easier to read. You can use the ternary operator for a one liner:

return full ? name + "(" + age + ")" : name;.


Your incrementAge() looks fine.


   public boolean canVote()
   {
     if(age >= 18)
     {
        return true;
     }
     else
     {
        return false;
     }
   }

Your indentation is weird. Use 4 spaces (less common tabs with width of 4 spaces) for indentation. You have sometimes 3 spaces, sometimes 2.

Since age >= 18 is a boolean expression, you can return the result just like this:

return age >= 18;

Use spaces after if and for (also while and similar) keywords. That makes the code clearer and more readable:

if (someCondition) {
    doThis();
}
\$\endgroup\$
0
\$\begingroup\$

My version of Person class would be,

public class Person
{
private int age;
private String name;

public Person(String st)
{
  this.name = st;
  this.age = 0;
}
public String toString(boolean full)
{
     return full?name + "(" + this.age + ")":name;
}

public int incrementAge()
{
   return ++this.age;
}

public boolean canVote()
{
    return (this.age >= 18); 
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ No StringBuilder in the toString method? \$\endgroup\$ – Gilbert Le Blanc Jul 6 '12 at 16:21
  • 1
    \$\begingroup\$ This is not a valid answer, as it is just a new version without any explaination or recommendations. Furthermore it contains many of the same mistakes as the original code. \$\endgroup\$ – Raimund Krämer Jan 19 '18 at 9:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy