Simple Hello World with unit tests

Question

This is a simple hello world sample project used for instruction. I'm just looking for general comments on the approach and design.

AdvancedHelloWorldTest.java

/**
 * AdvancedHelloWorldTest.java
 * 2014 GetGnosis.com
 */
package com.getgnosis.tutorials.tutorial01001;

import static org.junit.Assert.*;

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;

import org.junit.Test;

/**
 *  Test Class for 
 *  {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld}
 *
 * @author mhemphill
 * @version 1.0.0 - Mar 9, 2014
 */
public class AdvancedHelloWorldTest {

    /**
     * Test method for 
     * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
     *  #AdvancedHelloWorld()}.
     */
    @Test
    public void testAdvancedHelloWorld() {
        try {
            new AdvancedHelloWorld();
        } catch (Exception e) {
            fail("Construction failed. ");
        }
    }

    /**
     * Test method for 
     * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
     *      #main(java.lang.String[])}.
     */
    @Test
    public void testMain() {
        final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
        System.setOut(new PrintStream(outContent));
        AdvancedHelloWorld.main(null);
        assertEquals("Hello World!" + System.getProperty("line.separator"), outContent.toString());
        System.setOut(null);
    }

    /**
     * Test method for 
     * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
     *      #toString()}.
     */
    @Test
    public void testToString() {
        String helloWorldString =  new AdvancedHelloWorld().toString();
        assertEquals("AdvancedHelloWorld [message=Hello World!]",helloWorldString);
    }

}

AdvancedHelloWorld.java

/**
 * AdvancedHelloWorld.java
 * 2014 GetGnosis.com 
 */
package com.getgnosis.tutorials.tutorial01001;

/**
 * This is an example program that prints the {@link String} "Hello World!" to
 * the system's output display.
 *
 * @author mhemphill
 * @version 1.0.0 - Mar 9, 2014
 */
public class AdvancedHelloWorld {

    static {
        message = "Hello World!";
    }

    /**
     * The {@link String} instance representing the message to be displayed.
     */
    private final static String message;    

    /**
     * The default constructor.  Initializes the value of message via the 
     * static block.
     */
    public AdvancedHelloWorld() {}

    /**
     * The application entry point.  Creates a new instance of
     * {@link AdvancedHelloWorld} and prints the value of the {@link String}
     * message to the system's output display.
     *  
     * @param args - @{link String} array representing the arguments to the
     * application.  Not used in this application.
     * 
     */
    public static void main(String[] args) {
        System.out.println(new AdvancedHelloWorld().getMessage());
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append("AdvancedHelloWorld [");
        if (getMessage() != null)
            builder.append("message=").append(getMessage());
        builder.append("]");
        return builder.toString();
    }

    /**
     * Returns {@link String} instance representing the message to be displayed. 
     * 
     * @return message - {@link String} instance representing the message to 
     * be displayed.
     */
    private final String getMessage() {
        return message;
    }

}

Answer 1

Having tests is great (I'd love to see more questions with tests), keep it up! Other parts I would keep simpler.

1. You might want this as a tag in your revision control system instead of commenting:

   * @version 1.0.0 - Mar 9, 2014
   

   This is also in the revision control history, so it's usually unnecessary:

   * @author mhemphill
   

   See: Pros and cons of versioning javadoc

2. I would not want to maintain comments on test methods like this:

   /**
    * Test method for 
    * {@link com.getgnosis.tutorials.tutorial01001.AdvancedHelloWorld
    *      #toString()}.
    */
   @Test
   public void testToString() {
       ...
   }
   

   I don't see why would it be useful. I haven't seen anyone who likes reading javadoc of test classes instead of the test code directly. The name of the method is good, it contains the main point: it's a test for toString, keep that and remove the superfluous comment.

3. @Test
   public void testMain() {
       final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
       System.setOut(new PrintStream(outContent));
       AdvancedHelloWorld.main(null);
       assertEquals("Hello World!" + System.getProperty("line.separator"), outContent.toString());
       System.setOut(null);
   }
   

   The System.setOut(null) statements should be in an @After tearDown method or at least in a finally block. If the assertEquals throws an exception (or something else) out won't be set back.

4. I the same testMain method I'd create a local variable for the expected result:

   final String expected = "Hello World!" + System.getProperty("line.separator");
   assertEquals(expected, outContent.toString());
   

   It's easier to read. (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

5. @Test
   public void testAdvancedHelloWorld() {
       try {
           new AdvancedHelloWorld();
       } catch (Exception e) {
           fail("Construction failed. ");
       }
   }
   

   The following is the same:

   @Test
   public void testAdvancedHelloWorld() {
       new AdvancedHelloWorld();
   }
   

   If the constructor throws an exception (or anything inside a @Test method) JUnit will tell you.

6. This comments does not say anything which isn't in the code already, I'd remove it:

   /**
    * The default constructor. Initializes the value of message via the static
    * block.
    */
   

   (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

7. static {
       message = "Hello World!";
   }
   
   /**
    * The {@link String} instance representing the message to be displayed.
    */
   private final static String message;
   

   The following is the same:

   private final static String message = "Hello World!";
   

   I would keep it simple.

8. @Override
   public String toString() {
       StringBuilder builder = new StringBuilder();
       builder.append("AdvancedHelloWorld [");
       if (getMessage() != null)
           builder.append("message=").append(getMessage());
       builder.append("]");
       return builder.toString();
   }
   

   The if condition can't be false (the message field is final and has a non-null value), so the following is the same:

   StringBuilder builder = new StringBuilder();
   builder.append("AdvancedHelloWorld [");
   builder.append("message=").append(getMessage());
   builder.append("]");
   return builder.toString();
   

9. I'd move the main method to a separate class. It's usually a good idea to separate a class from its clients.

Answer 2

Despite my comment, I'll try to give a bit of a review. It is difficult though - a Hello World application will never need to be extended, and has such an extremely simple function that it's very hard to distinguish between a good and bad test.

Unnecessary comments

Generally, for comments within methods, I view them as a last resort. If I have to add a comment, it means the code itself isn't readable, which I treat as a code smell.

For Javadoc comments, I'm a bit looser. It's often just plain not possible to communicate everything you need to in a message signature. Having said that, there is such a thing as too much here too. If you write something that's just a simple restatement of what's already available in the signature, you're repeating yourself. As violations of the Don't Repeat Yourself principle go, it's a relatively benign one, but it's still a waste of everyone's time, and still means that you now have two places to maintain if that signature changes, rather than one.

So for example in your code you have:

/**
 * Returns {@link String} instance representing the message to be displayed. 
 * 
 * @return message - {@link String} instance representing the message to 
 * be displayed.
 */
private final String getMessage() {
    return message;
}

Is any of this necessary? You don't need to say it's a string, it's right there in the signature. And everyone knows what a string is so linking to it is useless. The name 'getMessage' already says that you're getting a message, so writing that again in the comment is pointless. I suppose you've added the information that this is the message "to be displayed", but that's rather vague, coming to this code for the first time it wouldn't tell me anything unless I then went and read the rest of the class. If you really think it's important to communicate the information that the message is being displayed, you could rename the variable and the corresponding get method instead.

And finally, the overall summary and the @returns part again say exactly the same thing.

This is a trap I fell into a lot myself. You think "Well, I have to write something for the Javadoc." But you really don't! If you have nothing to say there, say nothing.

Use of static

This is where we get into "difficult to review because it's such a simple application" territory. Because you're basically done now, you're not going to extend this or use it in a larger application or anything, there's nothing really wrong with using a static message as you have. So I'll try to talk about why, in a more general situation, it might not be a good idea.

Essentially I don't really know why you'd make the message static and everything else instance. Why not get rid of your static message field altogether and just have:

private final String getMessage() {
    return "Hello World!";
}

It's less code, just as readable, and no less flexible or extensible than what you have.

If you imagine some possible extensions to this class- maybe pass a second string into the constructor which would get added onto the end of your "Hello World!" message, or a public reverseMessage(), you'll quickly realise it either makes sense for all of it to be static or none of it. If there were going to be multiple AdvancedHelloWorld instances, there's no reason they'd all want to be looking at the same message string.

Answer 3

This must be one of the most complicated Hello World! implementations I have seen in a while.

I object to this being called a hello-world problem at all... by definition (wikipedia) a hello world program is supposed to illustrate the simplest way to do it.... and is not supposed to be used to illustrate complex processes. I would call your program, and the text it outputs, as something else.... based on what you are trying to teach... but I can't figure out what that is. Are you using this as an example of:

• Code Style
• static initializers
• JUnit testing
• StringBuilder usage
• getters ...

because I cannot figure it out.

Code Style

All static-final fields should have CAPITAL names.

The static field should be:

    private final static String MESSAGE; 

Static initializer block

As an example of a static initializer, I don't like it...

it may not make sense off-hand, but static initialize blocks should appear after the declaration for the variables they declare.

The order of execution of a static initializer is complicated, and the best way to ensure it works is to write it out in the same order that you would expect it to run. In your case the code works, but, in more complicated cases, your code may not work.... for example, what would you expect the following code to output?

public class StaticBlocks {
    
    static String hello;
    static String bilbo = "junk";

    static {
        hello = "Hello";
        bilbo = "Bilbo";
        baggins = "Baggins";
    }

    static String baggins = "junk";


    public static void main(String[] args) {
        System.out.println("Output: " + hello + " " + bilbo + " " + baggins);
    }
    
}

If you expected Output: Hello Bilbo Baggins, then you would be wrong..... because the output is Output: Hello Bilbo junk.

To avoid static initializer surprises I strongly recommend:

• don't do them at all.... (there are some exceptions), instead have static methods that produce the right results if you have complicated values to initialize.
• always structure the code as declarations first, followed by initializer static blocks so that the static block runs after any other initialization is done.

Junit Testing

Testing a main method is not something I have seen before. Interesting.

On the other hand, in this case, why? You should be testing the toString() method and the getMessage() method. You are not testing getMessage() directly at all, but relying on main to do that test work for you.

also, re-assigning the System.out is a really bad idea. If the test does fail, you don't complete the re-assign. Also, it is not thread-safe, and leaving System.out as null is not exactly friendly..... :(

StringBuilder usage

Others have suggested there is no reason for the null-check on the get-message.

I agree, there is no reason/way for the getMessage() to return null... and, why are you calling it twice? You should just kill the null-check.

getters

Why do you have an instance getter that retrieves a static field? This is broken.

The method is private, and it returns a static constant field. The method should not exist at all, and all references to getMessage() should be replaced with MESSAGE