4
\$\begingroup\$

How can I optimize this code to have fewer loops and return values for unit testing?

 public class Diamond {

public void DiamondShape(int num) {

   for(int ucount=num;ucount>0;ucount--) {
//Loop to print blank space
    for(int count_sp=1;count_sp<ucount;count_sp++)
        System.out.printf(" ");
//Loop to print *
    for(int count_ast=num;count_ast>=ucount;count_ast--)
        System.out.printf("* ");
    System.out.println();
   }

//Loop for lower half
  for(int lcount=1;lcount<num;lcount++) {
    //Loop to print blank space
    for(int count_sp=0;count_sp<lcount;count_sp++)
            System.out.printf(" ");
    //Loop to print *
        for(int count_ast=num-1;count_ast>=lcount;count_ast--)
            System.out.printf("* ");
    System.out.println();
    }
  } 
}

Output when num = 3:

   *
  * *
 * * *
  * *
   *

This is how the output should be. num indicates the star in center line.

\$\endgroup\$
5
\$\begingroup\$

First of all: Use a StringBuilder instead of System.out.println so you can easily compare the result of your method with an expected output.

Your test could look like: (I renamed you method to draw and moved the size to the instance.)

@Test
public void test4()
{
    StringBuilder expected = new StringBuilder();
    expected.append( "   * \n" );
    expected.append( "  * * \n" );
    expected.append( " * * * \n" );
    expected.append( "* * * * \n" );
    expected.append( " * * * \n" );
    expected.append( "  * * \n" );
    expected.append( "   * \n" );
    assertEquals( expected.toString(), new Diamond( 4 ).draw() );
}

After you have all your tests (I have just created some for 0-4) you can try to extract some methods with duplicate code.

public class Diamond
{
    private int size;

    public Diamond( int size )
    {
        this.size = size;
    }

    public String draw()
    {
        StringBuilder result = new StringBuilder();
        for( int ucount = size; ucount > 0; ucount-- )
        {
            appendSpaces( result, ucount - 1 );
            appendStars( result, size - ucount + 1 );
            newLine( result );
        }

        for( int lcount = 1; lcount < size; lcount++ )
        {
            appendSpaces( result, lcount );
            appendStars( result, size - lcount );
            newLine( result );
        }
        return result.toString();
    }

    private void newLine( StringBuilder result ) // just for better readability
    {
        result.append( "\n" );
    }

    private void appendStars( StringBuilder result, int count ) // just for better readability
    {
        repeat( result, "* ", count );
    }

    private void appendSpaces( StringBuilder result, int count ) // just for better readability
    {
        repeat( result, " ", count );
    }

    private void repeat( StringBuilder result, String string, int count )
    {
        for( int c = 0; c < count; c++ )
            result.append( string );
    }
}

If you want to get rid of one of the loops you could even merge it to:

public String draw()
{
    StringBuilder result = new StringBuilder();
    for( int ucount = size; ucount >= -size; ucount-- )
    {
        boolean isMiddleRows = ucount == 0 || ucount == -1;
        if( isMiddleRows ) continue;
        appendSpaces( result, Math.abs( ucount ) - 1 );
        appendStars( result, size - Math.abs( ucount ) + 1 );
        newLine( result );
    }
    return result.toString();
} 

It's pretty cool to have enough tests do try such refactoring :)

\$\endgroup\$
  • \$\begingroup\$ What other tests can we perform in this program \$\endgroup\$ – Ajit Apr 17 '13 at 5:22
  • \$\begingroup\$ I think checking the output for 0 to 4 is enough. Maybe you can test negative values, but in general you just need to find all border cases. -1, 0, 1, odd(3), even(4), maybe some larger number too. \$\endgroup\$ – mheinzerling Apr 17 '13 at 5:43
  • \$\begingroup\$ I am raising an exception if value is negative or a string so can we create a test case for the exception that I am raising \$\endgroup\$ – Ajit Apr 17 '13 at 11:27
  • \$\begingroup\$ stackoverflow.com/questions/3083161/junit-exception-testing \$\endgroup\$ – mheinzerling Apr 17 '13 at 11:43
  • \$\begingroup\$ how can i test methods appendStars() appendSpaces() and repeat() \$\endgroup\$ – Ajit Apr 19 '13 at 7:08
1
\$\begingroup\$

If you want to unit test the System.out output, you can set the PrintStream of System.out by using the setOut method.

private final ByteArrayOutputStream outContent = new ByteArrayOutputStream();

@Before
public void setUpStreams() {
    System.setOut(new PrintStream(outContent));
}

@After
public void cleanUpStreams() {
    System.setOut(null);
}

@Test
public void testOut() {
    System.out.println("hello");
    System.out.println("hi");
    String[] linesOfOutput = outContent.toString().split(System.getProperty("line.separator"));
    assertEquals("hello", linesOfOutput[0]);
    assertEquals("hi", linesOfOutput[1]);
}

Of course, it would be better to pass an OutputStream as parameter to the method and write to that.

public void writeDiamondShape ( int num, OutputStream outputStream ) {
    ...
}

As for optimization

public static void writeDiamond(int num){
    int loops = num * 2 - 1;
    int stars = 1;
    for ( int i = 0; i < loops; i++ ) {

        // Print spaces
        int spaces = Math.abs(num - i - 1);
        for ( int j = 0; j < spaces; j++ ) {
            System.out.print ( " " );
        }

        // Print stars
        for ( int j = 0; j < stars; j++ ) {
            System.out.print ( "* " );
        }
        System.out.println();

        // Increment / Decrement stars to print next time
        stars += (i+1 < num) ? 1 : -1;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I think you should definitely go with the parameter solution if you don't want to use a string return. Changing in, err and out might lead do unexpected behavior, e.g. if you debug a failing test by System.out.print(). \$\endgroup\$ – mheinzerling Apr 15 '13 at 11:07

Your Answer

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

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