25
\$\begingroup\$

I just yesterday started learning some Java. I wrote this piece and got some help from @Simon André Forsberg to refine it some. I added a few things since. It's super-simple but I just want to make sure I have a few concepts down before I move on to more complex things. Any constructive criticism is appreciated!

import java.lang.*;

public class HelloWorld {
    private final String output;

    public HelloWorld(String name) {
        output = "Hello World, " + name + "!";
    }

    public String getMessage() {
        return output;
    }

    public static void main (String args[]) {
        HelloWorld helloObj;
        helloObj = new HelloWorld("Phrancis");
        System.out.println(helloObj.getMessage());
    }
}

Console output:

Hello World, Phrancis!
\$\endgroup\$

5 Answers 5

23
\$\begingroup\$

In Java it is seldom necessary to create void variable 'declarations'. The code:

HelloWorld helloObj;
helloObj = new HelloWorld("Phrancis");

should just be:

HelloWorld helloObj = new HelloWorld("Phrancis");

Then, for the actual HelloWorld class, there are a few hints:

  1. All Java objects have a toString() method that should, in most cases, be overridden to provide useful diagnostics, or other information. In this case, it would make sense to use the toString() instead of getMessage()
  2. You should store data in it's 'raw' form when possible. Your HelloWorld class should store the 'name' and not the 'output'.
  3. You should provide getters for internal properties.

I would write the class as:

public class HelloWorld {
    private final String name;

    public HelloWorld(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }

    @Override
    public String toString() {
        return "Hello World, " + name + "!";
    }

}

Then your main method could be:

public static void main (String args[]) {
    HelloWorld helloObj = new HelloWorld("Phrancis");
    System.out.println(helloObj);
}
\$\endgroup\$
5
  • 24
    \$\begingroup\$ I personally don't like how toString() is used as business logic instead of debugging purposes. It's like two worlds are intertwining. A separate getMessage() method would have my preference because it is also more self-documenting than suddenly printing out an object. \$\endgroup\$ Dec 3, 2014 at 18:25
  • 10
    \$\begingroup\$ "You should provide getters for internal properties." - Not necessarily. It depends on what the object's purpose is. \$\endgroup\$
    – oconnor0
    Dec 3, 2014 at 19:27
  • 2
    \$\begingroup\$ Didn't you yourself, rolfl, say in an answer earlier this year that toString() should not be used for user output but only as debugging information? \$\endgroup\$ Dec 3, 2014 at 22:22
  • 3
    \$\begingroup\$ @SimonAndréForsberg - What I said was not so clear-cut. I said that it should be documented whether the toString had more of a contract than just debug, but you should always have a toString.... This is the answer you are referring to \$\endgroup\$
    – rolfl
    Dec 3, 2014 at 22:35
  • 5
    \$\begingroup\$ @JeroenVannevel I would argue that the exception to that rule is when the class represents a string-like thing, like a StringBuilder, or some sort of character array. In that case, toString() is a logical choice for getting the resultant string. \$\endgroup\$ Dec 4, 2014 at 9:36
26
\$\begingroup\$

First things first:

import java.lang.*;

This import is superfluous. Java by default imports the java.lang package, you don't have to specify any further things.


You seem to be using consistent two newlines between last } and the next opening brace. I personally find this to take a lot of space and I'd reserve "large" vertical distance to signify sectioning or similar, it's consistent ;)


public static void main (String args[]) {

While this is a perfectly valid declaration for main, I'd prefer to have the array declaration behind the type and not behind the argument name for consistency's sake:

public static void main (String[] args) {
\$\endgroup\$
1
22
\$\begingroup\$

Instead of string concatenation, another alternative is using String.format(...) for formatting the message. Combining suggestions from other answers, I'd go with this implementation:

public class HelloWorld {
    private final String message;

    public HelloWorld(String name) {
        this.message = String.format("Hello World, %s!", name);
    }

    @Override
    public String toString() {
        return message;
    }
}

Since name doesn't change, it's good to pre-format the message at construction time, so that toString can simply return it directly.

If you want to access the message directly, you could add a getter for it.

If you want to access the name, you might add a field and a getter for it. But probably a "HelloWorld" object doesn't really need any accessors, does it?

UPDATE

The reason I suggested String.format is because it was not mentioned by other answers, and it's often a useful alternative. It may be a matter of taste, a benefit of this approach is that a template string with placeholders + parameter list can be more readable than concatenation.

As per my discussion in comments with @vaxquis, be aware that the output of String.format may depend on locale, for example when formatting dates and numbers. There is another version of String.format(Locale, String, ...) that takes the Locale as the first parameter to ensure predictable output. (Without this parameter, the default locale is used.) This is especially important in applications intended to run in multiple locales, such as Android apps. For this reason, Android linters flag a warning for uses without a locale parameter.

In this particular example, there are no such concerns, because we're not printing anything locale-sensitive (no dates, numbers). But it's good to be aware of this locale-sensitive behavior in situations where it can make a difference.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ @vaxquis Since we're only formatting string parameters in this very simple example, I don't really agree that the detailed discussion about formatting numbers and dates and locale-sensitive situations like Android belong here. But anyway, I added an update explaining these concerns. \$\endgroup\$
    – janos
    Dec 4, 2014 at 20:16
  • 1
    \$\begingroup\$ since we're only formatting String parameters, I don't see any need to use String.format() vs simple + concatenation. Still, you've a valid point about format() being a useful alternative; I'm grateful for your update and, because your answer is now verbose, covering the caveats, and thus IMO useful, you get a well-deserved up from me. \$\endgroup\$
    – user20300
    Dec 4, 2014 at 21:55
  • 1
    \$\begingroup\$ @vaxquis Glad you like the update! (I wish you hadn't deleted your last few comments though. You've made some good points, and the links were especially helpful.) Thanks for your open-minded attitude, and indeed, I too will be more careful with String.format in the future! \$\endgroup\$
    – janos
    Dec 4, 2014 at 22:03
16
\$\begingroup\$
private final String output;

I like this. Initializing private fields from constructor arguments, and making them final sounds exactly like The Right Thing™.

public static void main (String args[]) {
    HelloWorld helloObj;
    helloObj = new HelloWorld("Phrancis");
    System.out.println(helloObj.getMessage());
}

This is a bit awkward though: you have written your (static) main method in the HelloWorld class, and then you have that main method create an instance of that very same class - I don't like that.

It's good that you're using objects - creating an instance of a class does that. But I would have implemented the parameterized constructor and the getMessage method in a separate class.

\$\endgroup\$
4
  • \$\begingroup\$ "main method create an instance of that very same class" - that's what I always do. Leaving the static context ASAP allows me to use instance members and methods and this is more refactoring-friendly than statics. \$\endgroup\$
    – maaartinus
    Dec 3, 2014 at 20:07
  • 2
    \$\begingroup\$ @maaartinus IMO the class that contains the main method should only serve as an entry point, and instantiate the actual app object with its dependencies; main instantiating the same class that has both a static and an instance context sounds like breaking SRP to me. \$\endgroup\$ Dec 3, 2014 at 20:22
  • \$\begingroup\$ My main is a one-liner (or close to) like new ThisClass().go() or alike using DI. So I obey SRP as there's practically no static context - if Java could call my non-static go directly, I'd use it. \$\endgroup\$
    – maaartinus
    Dec 3, 2014 at 22:19
  • 1
    \$\begingroup\$ Only answer mentioning this oddity? +1 \$\endgroup\$
    – nhgrif
    Dec 5, 2014 at 12:34
14
\$\begingroup\$

For readability property name and corresponding methods name should be relative.

private final String message; 

public String getMessage() {
    return message;
}

I suggest you to use StringBuilder for String concatenation.

StringBuilder builder = new StringBuilder();
builder.append("Hello").append(name).append("!");
output = builder.toString();
\$\endgroup\$
7
  • 9
    \$\begingroup\$ Isn't string builder over kill for something like this? Java has a string format type thing doesn't it? \$\endgroup\$
    – RubberDuck
    Dec 3, 2014 at 17:56
  • 2
    \$\begingroup\$ @RubberDuck You're right. In this case String.format is the best option. \$\endgroup\$ Dec 3, 2014 at 18:13
  • 6
    \$\begingroup\$ There is absolutely nothing wrong with concatenating strings with +. StringBuilders are advised when you don't know the amount of concatenations like in a loop. Using a StringBuilder here clutters the code and adds nothing of value. A formatter might be fun to introduce the concept. \$\endgroup\$ Dec 3, 2014 at 18:19
  • 3
    \$\begingroup\$ @RubberDuck String.format is nice, but slower. Here, obviously nothing matters. StringBuilder is completely pointless here, as + does exactly the same (it makes sense only to eliminate intermediate string creation and there's none here). \$\endgroup\$
    – maaartinus
    Dec 3, 2014 at 20:05
  • \$\begingroup\$ What exactly do you mean by property name and corresponding methods name should be relative ? (Edit: You mean the name, right? In that case I agree). \$\endgroup\$ Dec 3, 2014 at 22:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.