patternjavaMinor

Shortening and optimizing diamond-printing code

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

shorteningdiamondcodeprintingoptimizingand

Problem

I'm working through a Java textbook by Paul and Harvey Deitel and I have come across a review question (Chapter 2 - Intro to Java, Exercise 2.18 for those that have the book) which asks the user to create the following diamond:

*    
   * *
  *   *
 *     *
*       *
 *     *
  *   *
   * *
    *

I have the following code which does this but, as always, I can't help but think there's a better way to do this. Any suggestions on a shorter way?

```
// Prints the start and end of the diamond
private static void printStartEnd(int numLines, char sign, char filler) {
int xPosition = numLines / 2;

// Loop through
for (int i = 0; i < numLines; i++) {
if (i == xPosition) {
System.out.print(sign);
} else {
System.out.print(filler);
}
}
}

// Prints the other lines
private static void printOtherLines(int numLines, int positionOne,
int positionTwo, char sign, char fill) {
int i = 0;

// Loop through
while (i < numLines) {
if (i == positionOne || i == positionTwo) {
System.out.print(sign);
} else {
System.out.print(fill);
}
i++;
}
}

// Draw the diamond to the console
public static void drawDiamond( int numLines, char sign, char fill )
{
// Diamonds can only be created for odd numbers
if( numLines % 2 != 0 )
{
boolean limitReached = false;

int halfWayPointMinus = numLines / 2 - 1;
int halfWayPointPlus = numLines / 2 + 1;

for( int i = 0; i < numLines; i++ )
{
if( i == 0 || i == numLines-1 )
{
printStartEnd( numLines, sign, fill );
}
else
{
printOtherLines( numLines,halfWayPointMinus,halfWayPointPlus,sign,fill );
if( halfWayPointMinus == 0 || limitReached )
{
limitReached = true;

// Invert values and move bac

Solution

Error handling

When you detect an error condition, it's usually better to put the error handler immediately after the if-condition, then bail out. The error handler is usually shorter than the main code, so it's less mental workload to get it out of the way instead of having to look for the matching else many lines further down. The advantage of putting the error handler first becomes more apparent when multiple errors are involved:

if (SUCCESS != validate1()) {
    return handleError1();
}
if (SUCCESS != validate2()) {
    return handleError2();
}
if (SUCCESS != validate3()) {
    return handleError3();
}
// The main processing can proceed here

… is more readable than:

if (SUCCESS == validate1()) {
    if (SUCCESS == validate2()) {
        if (SUCCESS == validate3()) {
            // Do the main processing here
        } else {
            handleError3();
        }
    } else {
        handleError2();
    }
} else {
    handleError1();
}

Secondly, it is usually more appropriate to print error messages to System.err. You don't want unexpected junk contaminating your output.

Consider changing the behaviour such that it throws an IllegalArgumentException rather than printing an error message. Throwing an exception allows the function's caller to programatically handle the error, making it more flexible and reusable.

Flow of Control

Using boolean variables (like limitReached) to control the flow of your program is usually a sign that you could structure the code better. In this case, you always take the else-branch when printing the top half of the diamond, then always take the if-branch for the bottom half. You might as well write it as two for-loops.

On the other hand, there is a competing Don't-Repeat-Yourself principle regarding the body of the loop. In my opinion, you would still be better off clarifying the flow-of-control by splitting it into two loops. To avoid copy-and-pasting the loop body, you want to push the special cases into your printing function, so that the body is just one function call. That would be a smart move anyway, as it makes your printing function more resilient and versatile.

Printing Function

It turns out that your printOtherLines() can be used to print the apex and the base just fine. You don't need printStartEnd() at all.

I would rename the numLines parameter to size or width. Calling it numLines in the context of printOtherLines() is a bit confusing, since you are only printing one line per function call.

Object-Oriented Thinking

I don't know whether your exercise specifies a particular interface, but I don't like drawDiamond(int numLines, char sign, char fill). A more object-oriented approach would be:

Diamond d = new Diamond(/* size= */ 5);
d.draw(System.out, '*', ' ');

In other words, the diamond knows how to draw itself.

Suggested Solution

import java.io.PrintStream;

public class Diamond {
    private int size;

    /**
     * Constructor.
     * @param size The size of the diamond in lines (or columns)
     * @throw IllegalArgumentException if size is negative or even
     */
    public Diamond(int size) {
        if (size  0) {
            // Handle the top half
            this.printLine(out, sign, fill, posL--, posR++);
        }
        while (posL <= posR) {
            // Handle the bottom half, including the middle (widest) line
            this.printLine(out, sign, fill, posL++, posR--);
        }
    }

    private void printLine(PrintStream out, char sign, char fill, int posL, int posR) {
        // Loop condition could be (col <= posR) to omit trailing filler
        for (int col = 0; col < this.size; col++) {
            out.print((col == posL || col == posR) ? sign : fill);
        }
        out.println();
    }

    public static void main(String[] args) {
        try {
            Diamond d = new Diamond(Integer.parseInt(args[0]));
            d.draw(System.out, '*', ' ');
        } catch (IllegalArgumentException e) {
            System.err.println("Sorry, the number of lines must be an odd number...");
        }
    }
}

Code Snippets

if (SUCCESS != validate1()) {
    return handleError1();
}
if (SUCCESS != validate2()) {
    return handleError2();
}
if (SUCCESS != validate3()) {
    return handleError3();
}
// The main processing can proceed here

if (SUCCESS == validate1()) {
    if (SUCCESS == validate2()) {
        if (SUCCESS == validate3()) {
            // Do the main processing here
        } else {
            handleError3();
        }
    } else {
        handleError2();
    }
} else {
    handleError1();
}

Diamond d = new Diamond(/* size= */ 5);
d.draw(System.out, '*', ' ');

import java.io.PrintStream;

public class Diamond {
    private int size;

    /**
     * Constructor.
     * @param size The size of the diamond in lines (or columns)
     * @throw IllegalArgumentException if size is negative or even
     */
    public Diamond(int size) {
        if (size <= 0) {
            throw new IllegalArgumentException("Diamond size must be positive");
        }
        if (size % 2 == 0) {
            throw new IllegalArgumentException("Diamond size must be odd");
        }
        this.size = size;
    }

    public void draw(PrintStream out, char sign, char fill) {
        int posL = this.size / 2,
            posR = posL;
        while (posL > 0) {
            // Handle the top half
            this.printLine(out, sign, fill, posL--, posR++);
        }
        while (posL <= posR) {
            // Handle the bottom half, including the middle (widest) line
            this.printLine(out, sign, fill, posL++, posR--);
        }
    }

    private void printLine(PrintStream out, char sign, char fill, int posL, int posR) {
        // Loop condition could be (col <= posR) to omit trailing filler
        for (int col = 0; col < this.size; col++) {
            out.print((col == posL || col == posR) ? sign : fill);
        }
        out.println();
    }

    public static void main(String[] args) {
        try {
            Diamond d = new Diamond(Integer.parseInt(args[0]));
            d.draw(System.out, '*', ' ');
        } catch (IllegalArgumentException e) {
            System.err.println("Sorry, the number of lines must be an odd number...");
        }
    }
}

Context

StackExchange Code Review Q#31734, answer score: 6

Revisions (0)

No revisions yet.