Print an ASCII diamond

Question

This takes a width specified by user and prints a diamond of that width. It uses only three for loops, but could I reduce that further? Is there a more elegant solution?

public class Diamond {

    static boolean cont = true;

    public static void main (String[] args) {
        Scanner input = new Scanner(System.in);
        while (cont) {
            System.out.print("Width: ");
            int width = input.nextInt();
            int lines = width;
            System.out.println();
            for (int line = 0; line < lines; line++) {
                for (int spaces = 0; spaces < Math.abs(line - (lines / 2)); spaces++) {
                    System.out.print(" ");
                }
                for (int marks = 0; marks < width - 2 * (Math.abs(line - (lines / 2))); marks++) {
                    System.out.print("x");
                }       
                System.out.println();
            }
            System.out.println();
        }
    }
}

Answer 1

There are a few things that we can do to clean this up.

• This is something I would extract to its own method. Handle getting the user input in the main() method, and then pass that on to the drawDiamond() method.

• Your for loops are divided into iterating over lines, spaces, and marks. We can simplify that down to just rows and columns where we can iterate over each individual unit at a time. This will also eliminate one of your System.out.println()s in the final method.

• We can simplify down the math of where to print a piece of the diamond.

  if ((column == Math.abs(row - half)) || (column == (row + half)) || (column == (sqr - row + half - 1)))
  

Final Method:

void drawDiamond(int sqr)
{
    int half = sqr/2;
    for (int row=0; row<sqr; row++)
    {
        for (int column=0; column<sqr; column++)
        {
            if ((column == Math.abs(row - half)) || (column == (row + half)) || (column == (sqr - row + half - 1)))
            {
                System.out.print("*");
            }
            else System.out.print(" ");
        }
        System.out.println();
    }
}

Answer 2

You know, it's nice to see code that does what it says, and a nice simple task that still requires some head scratching.... but I'll assume you're a Java beginner.

Basics

Going through some of the basic stuff...

• you have the cont variable declared as a static variable outside the method, but the only place it is used is inside the method. In this case, you should move the declaration inside the main method. Also, nothing changes that state, so the program just runs, and runs, which is OK (as a beginner).
• you do not close the input Scanner. Again, this is probably because the program never completes, but there are nice ways in Java7 to make sure it happens neatly, and without much effort.
• you should probably validate the user input. If the user enters negative integers, it's actually OK (the program does nothing). More concerning is if the user enters 2000000000. You should set an upper bound.
• you call this shape a 'diamond' but it is actually a square. The width and height are the same number of characters. You only need one variable, lines or width, not both.
• it is confusing that you have both line and lines variable. There is no need for lines if you use width instead, so get rid of it (also since the user prompt is "Width:").

OK, that's some relatively simple stuff. Messing your code around using the above suggestions I get:

public static void main (String[] args) {
    boolean cont = true;

    try (Scanner input = new Scanner(System.in)) {
        while (cont) {
            System.out.print("Width: ");
            int width = input.nextInt();
            System.out.println();

            if (width > 100) {
                System.out.println("Width too wide, reducing to 100");
                width = 100;
            }

            for (int line = 0; line < width; line++) {
                for (int spaces = 0; spaces < Math.abs(line - (width / 2)); spaces++) {
                    System.out.print(" ");
                }
                for (int marks = 0; marks < width - 2 * (Math.abs(line - (width / 2))); marks++) {
                    System.out.print("x");
                }       
                System.out.println();
            }
            System.out.println();
        }
    }
}

Algorithm

OK, now, some algorithmic things:

• System.out.print(...) and the println variants, are actually really slow. Calling them from inside loops is a real problem for performance, and is a bad habit to learn. These methods lock the console output, and are not nice to other threads either. Where possible, you should always batch up the character printing in to a larger statement.
• Sometimes, taking stuff away is easier than adding it .... (cryptic hint).

We can solve a lot of the complexity in your loops by doing a couple of tricks. Here's a suggestion:

1. Build up two Strings, one of spaces and the other of 'x' characters. Each should be at least as long as the longest value we will need.
2. loop through the rows and use parts of each of the two above strings.

The logic is the exact same as yours except I have big things I use a part of, whereas you build it up bit by bit. The important part is the Math.abs(...) statements are identical to the previous version.... they are the limit to the values we build.

Here's a way to do it:

public static void main (String[] args) {
    boolean cont = true;

    try (Scanner input = new Scanner(System.in)) {
        while (cont) {
            System.out.print("Width: ");
            int width = input.nextInt();
            System.out.println();

            if (width > 100) {
                System.out.println("Width too wide, reducing to 100");
                width = 100;
            }

            char[] spaces = new char[width / 2];
            char[] exes = new char[width];
            Arrays.fill(spaces, ' '); // now an array of spaces
            Arrays.fill(exes, 'x'); // now an array of 'x'

            for (int line = 0; line < width; line++) {
                String pad = new String(spaces, 0, Math.abs(line - (width / 2)));
                String fill = new String(exes, 0, width - 2 * (Math.abs(line - (width / 2))));
                System.out.println(pad + fill);
            }
            System.out.println();
        }
    }
}

That's just something for you to think about.....

Answer 3

People have gone into lots of important things about syntax, program structure, etc. But I think that you could make your loops much simpler and easier to understand:

int halfheight = (width + 1) / 2;
int spaces = halfheight;
int exes = width - 2 * spaces;

for (int i = 0; i < halfheight; i++)
{
    spaces--;
    exes += 2;

    // You could use the approaches suggested by other folk here instead of inner loops
    for (int s = 0; s < spaces; s++)
        System.out.print(" ");
    for (int x = 0; x < exes; x++)
        System.out.print("X");
    System.out.print("\n");
}

for (int i = 0; i < halfheight - 1; i++)
{
    spaces++;
    exes -= 2;

    for (int s = 0; s < spaces; s++)
        System.out.print(" ");
    for (int x = 0; x < exes; x++)
        System.out.print("X");
    System.out.print("\n");
}

(I believe that this handles odd and even widths exactly the same as yours, except that it doesn't output an initial blank line in the case of even width. I assumed this was an artefact rather than part of the spec.)

The only 'math' I do is in the initialization, then I loop by amounts which are completely clear without any calculation, and change the number of exes and spaces in each iteration in a way that is completely obvious. It may seem that the various calculations with abs and integer division are not complex, however when you are debugging or trying to slightly modify your code, you will waste time on thinking through different cases (positive, negative, odd, even, first iteration, last iteration), and will probably find yourself just testing different values something, something + 1, something - 1 etc, rather than being able to see the precise interaction of each variable at the edge cases.

Note that I have 6 for loops and the code is much longer than the corresponding parts of yours or anyone else's. It's not reducing these that makes the code clear. It's the fact that it is easy to grasp what each does without referring to (much) stuff outside of the loop that helps the maintainer. IMHO (and I know some would disagree), repetition used sparingly and symmetrically as it is here is more elegant than it would be if I factored it out.

Answer 4

It's bad practice to put all of your code into main(). Here, you have the additional problem that main() does three things: prompt for input, print the diamond, and loop. (By the way, you offer no error-free way to exit from the infinite loop.) Mixing these tasks into one function would make it impossible, for example, to reuse your diamond-printing routine in some other way (such as making a continuous vertical string of several diamonds).

@syb0rg offered one way to split the input routine from the printing routine. I would go further and suggest that an object-oriented interface would be a good habit to build in Java. Here's one way:

private static int promptWidth(Scanner input) {
    System.out.print("Width: ");
    return input.hasNextInt() ? input.nextInt() : 0;
}

public static void main(String[] args) {
    Scanner input = new Scanner(System.in);
    int width;
    // Exit cleanly on EOF, or if anything other than a positive
    // integer is entered.
    while ((width = promptWidth(input)) > 0) {
        System.out.println();
        new Diamond(width).draw(System.out);
        System.out.println();
    }
    input.close();
}

In other words, a Diamond knows how to draw itself to System.out.

Here's another approach:

public static void main(String[] args) {
    Scanner input = new Scanner(System.in);
    int width;
    // Exit cleanly on EOF, or if anything other than a positive
    // integer is entered.
    while ((width = promptWidth(input)) > 0) {
        int width = input.nextInt();
        System.out.println();
        System.out.println(new Diamond(width));
        System.out.println();
    }
    input.close();
}

That relies on Diamond's .toString() method, which you would have to implement using a StringBuilder.

Answer 5

You can indeed do it in one loop. As I don't speak Java, so I'll use C# syntax.

public static void Main()
{
    string valueString;
    int width;
    do
    {
        Console.Write("Width:");
        valueString = Console.ReadLine();
    } while (!int.TryParse(valueString, NumberStyles.Integer, CultureInfo.InvariantCulture, out width) && width > 0 && width <= 100);
    int half = (int)((double)width/2+0.5);
    string pattern = new string(' ',width)+new string('*',width);
    for (int row = 1; row <= width; row++)
    { 
        int spaces = width-Math.Abs(half - row);
        Console.WriteLine(pattern.Substring(spaces, spaces));
    }
}

---

Translated to Java for your convenience ....

public static void main (String[] args) {

    try (Scanner scanner = new Scanner(System.in)) {
        int width;
        do
        {
            System.out.print("Width:");
            width = scanner.nextInt();
        } while (width < 0 || width > 100);

        char[] blanks = new char[width];
        char[] exes = new char[width];
        Arrays.fill(blanks, ' ');
        Arrays.fill(exes, 'x');
        String pattern = new String(blanks) + new String(exes);

        int half = (int)((double)width / 2 + 0.5);
        for (int row = 1; row <= width; row++)
        {
            int spaces = width - Math.abs(half - row);
            // Java substring has arguments (first, last), not (first, length).
            System.out.println(pattern.substring(spaces, spaces + spaces));
        }                

        System.out.println();
    }
}