Shortening and optimizing diamond-printing code

Question

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 back in
 halfWayPointMinus++;
 halfWayPointPlus--;
 }
 else
 {
 halfWayPointMinus--;
 halfWayPointPlus++;
 }
 }
 // Move to next line
 System.out.println();
 }
 } else
 {
 System.out.println( "Sorry, the number of lines entered must be an odd number..." );
 }
}

Answer 1

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) {
 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...");
 }
 }
}

• Katana24: I would have one question (at the moment) on your implementation; in the main method you surround the declaration of the class and subsequent call to its Draw method in a try-catch block and catch the err - why do this when the constructor handles this error anyway? Surely its redundant code?
• 200_success: The constructor complains. The main method handles the complaint. It would be inappropriate for the constructor to handle the error all on its own — the illegal argument should cause the Diamond object to fail to be created. It would also be inappropriate for the constructor to do System.err.println(); System.exit(); since the constructor's sole job should be to create an object; it shouldn't unilaterally decide to bring down the JVM. Therefore, it just throws an exception and lets the caller deal with it.

Answer 2

You can do away with the printStartEnd method, as the printOtherLines can do the same job, if you send in the same value for positionOne and positionTwo.

If you have a numeric value for direction instead of a boolean flag, you can use the same code for both movements.

 int halfWayPointMinus = numLines / 2;
 int halfWayPointPlus = halfWayPointMinus;
 int dir = -1;
 for (int i = 0; i < numLines; i++) {
 printOtherLines(numLines, halfWayPointMinus, halfWayPointPlus, sign, fill);
 if (halfWayPointMinus == 0) {
 dir = 1;
 }
 halfWayPointMinus += dir;
 halfWayPointPlus -= dir;
 System.out.println();
 }