20
\$\begingroup\$

I'm going through a beginners exercise for writing a small program in Java. My task is to print out (i.e. find an algorithm) the following output:

XOOOOOOOOO
XXOOOOOOOO
XXXOOOOOOO
XXXXOOOOOO
XXXXXOOOOO
XXXXXXOOOO
XXXXXXXOOO
XXXXXXXXOO
XXXXXXXXXO
XXXXXXXXXX

I have figured out how to do it but the code I wrote seems repetitive and tedious. How can I shorten my code?

package helloWorld;

public class HelloWorld {

public static void main(String[] args) 
{
    // Output


    // Algorithm 1

    boolean ft = true;

    String s = new String();
    for(int i = 0; i < 10; i++)
    {
        s  += "X";
        for(int j = 0; j < 9; j++)
        {
            if(i == 0)
            {

                s += "O";
            }

            if(i == 1)
            {

                if(ft == true)
                {
                    s+="X";
                    ft = false;
                }
                if(j == 8)
                {
                    continue;
                }
                s += "O";
            }

            if(i == 2)
            {

                if(ft == true)
                {
                    s+= "XX";
                    ft = false;

                }

                if(j == 7)
                {

                    break;
                }
                s += "O";

            }

            if(i == 3)
            {

                if(ft == true)
                {
                    s+= "XXX";
                    ft = false;

                }

                if(j == 6)
                {

                    break;
                }
                s += "O";
            }

            if( i == 4)
            {
                if(ft == true)
                {
                    s+= "XXXX";
                    ft = false;

                }

                if(j == 5)
                {

                    break;
                }
                s += "O";
            }

            if( i == 5)
            {
                if(ft == true)
                {
                    s+= "XXXXX";
                    ft = false;

                }

                if(j == 4)
                {

                    break;
                }
                s += "O";
            }

            if( i == 6)
            {
                if(ft == true)
                {
                    s+= "XXXXXX";
                    ft = false;

                }

                if(j == 3)
                {

                    break;
                }
                s += "O";
            }

            if( i == 7)
            {
                if(ft == true)
                {
                    s+= "XXXXXXX";
                    ft = false;

                }

                if(j == 2)
                {

                    break;
                }
                s += "O";
            }

            if( i == 8)
            {
                if(ft == true)
                {
                    s+= "XXXXXXXX";
                    ft = false;

                }

                if(j == 1)
                {

                    break;
                }
                s += "O";
            }

            if( i == 9)
            {
                if(ft == true)
                {
                    s+= "XXXXXXXXX";
                    ft = false;

                }

                if(j == 0)
                {

                    break;
                }
                s += "O";
            }


        }
        System.out.println(s);
        s = "";
        ft = true;

    }
}

}
\$\endgroup\$

We are looking for answers that provide insightful observations about the code in the question. Answers that consist of independent solutions with no justification do not constitute a code review, and may be removed.

  • 3
    \$\begingroup\$ Divide the problem in simpler parts. First just think about the first row. Look for a common pattern. 10 rows, 10 symbols on each row, and one increasing while other is decreasing, so kind of COMPLIMENTARY. \$\endgroup\$ – piepi Sep 2 '16 at 6:06
31
\$\begingroup\$

Lots of answers already, but most of them seem to be about alternative solutions. I'll review your code on the details and show you how to refactor your code step by step. (This is a long answer, link to bottom of answer)


String s = new String();

Creating a new String could be simplified to "". The only time you'll actually use the String constructor is for creating strings from arrays like char[] or byte[].

The first line of output

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        if(i == 0)
        {

            s += "O";
        }

Seems simple enough. Add an X, then add 9 O's.

The second line of output

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {

        if(i == 1)
        {

            if(ft == true)
            {
                s+="X";
                ft = false;
            }
            if(j == 8)
            {
                continue;
            }
            s += "O";
        }

ft starts off true. Like that, you'll print 1 X, then enter the j loop, add one more X, then an O, and you loop from there to add the remaining characters. This leads to the problem that you have one character extra... so you have to add the j == 8 check to stop the algorithm from printing a line with 10 characters.

You can check boolean conditions without an equality operator, like so:

if(ft)
{
    s+="X";
    ft = false;
}

This will work just as well as the ft == true check.

The third line of output

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {

        if(i == 2)
        {

            if(ft == true)
            {
                s+= "XX";
                ft = false;

            }

            if(j == 7)
            {

                break;
            }
            s += "O";

        }

Same as the second line, except here you add two X on the first iteration, and to compensate, you break one iteration earlier. This is kind of problematic, since you now have to duplicate your code for each iteration. You're missing out on the power of the for-loop.

Did you know Strings come with a length() method? You can retrieve the current length with it.

We can use that method to change the for loop so that each iteration adds just one character.

        if(i == 2)
        {

            if(ft == true)
            {
                s+= "XX";
                ft = false;

            }

            if(j == 7)
            {

                break;
            }
            s += "O";

        }

If we use the length() method here to determine ft...

        if(i == 2)
        {
            if(ft == true)
            {
                s+= "XX";
                ft = s.length() < 3;
            }

            if(j == 7)
            {

                break;
            }
            s += "O";

        }

Then we can change the code so that it only adds one character at a time, removing the condition that was added to prevent going over 10 characters.

        if(i == 2)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < 3;
            } 
            else
            {
                s += "O";
            }
        }

We could copy this change for all cases...

        if(i == 3)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < 4;
            } 
            else
            {
                s += "O";
            }
        }

but that seems like we're still duplicating code.

Fortunately, it seems we can make use of the fact that... if i == n, then the check needs to be s.length() < n + 1.

So let's apply that change.

        if(i == 3)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            } 
            else
            {
                s += "O";
            }
        }

Now all the cases look like this:

        if(i == 2)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            } 
            else
            {
                s += "O";
            }
        }

        if(i == 3)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            } 
            else
            {
                s += "O";
            }
        }

Seems like we can merge the cases i == 1 through i == 9 because they're all the same. And since i only goes to 9 because of the i < 10 condition, all we're interested in is checking that it's not the first iteration:

        if(i != 0)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            } 
            else
            {
                s += "O";
            }
        }

Here's where we're at now.

boolean ft = true;

String s = "";
for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        if(i == 0)
        {

            s += "O";
        }

        if(i != 0)
        {
            if(ft)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            } 
            else
            {
                s += "O";
            }
        }

    }
    System.out.println(s);
    s = "";
    ft = true;

}

Now... if we modified the setting of ft to do the s.length() < (i + 1) check before we tried to add a new character to the line, we could even merge the i == 0 case.

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        ft = s.length() < i;
        if(i == 0)
        {

            s += "O";
        }

        if(i != 0)
        {
            if(ft)
            {
                s += "X";
            } 
            else
            {
                s += "O";
            }
        }

    }
    System.out.println(s);
    s = "";
    ft = true;

}

Now, in the case i == 0, ... you enter the j-for loop with one "X" in your string already. So ft would be set to false, always. And in the i != 0 case, if ft is not true... we do the same thing as the i == 0 case. So we can remove the checks whether i is or is not 0, as it will work just fine:

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        ft = s.length() < i;
        if(ft)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
    s = "";
    ft = true;

}

Now that we've made that change, the code is short. But it's not that clear yet. ft is a pretty meaningless variable. If we renamed it to notEnoughXYet, the code becomes a bit more clearer:

boolean notEnoughXYet = true;

String s = "";
for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
    s = "";
    notEnoughXYet = true;

}

... We can still go further, though.

There are two places we set notEnoughXYet to true and s to "". Maybe we can merge them?

What about putting it at the start of each iteration of the i for loop?

for(int i = 0; i < 10; i++)
{
    boolean notEnoughXYet = true;

    String s = "";
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
    s = "";
    notEnoughXYet = true;

}

Like this, we see that we can eliminate some more duplication: the setting of s and notEnoughXYet at the bottom can disappear. And s could be initialized to X.

for(int i = 0; i < 10; i++)
{
    boolean notEnoughXYet = true;
    String s = "X";
    for(int j = 0; j < 9; j++)
    {
        notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

You could even move notEnoughXYet to the inner for loop...

for(int i = 0; i < 10; i++)
{
    String s = "X";
    for(int j = 0; j < 9; j++)
    {
        boolean notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

Or get rid of it entirely.

for(int i = 0; i < 10; i++)
{
    String s = "X";
    for(int j = 0; j < 9; j++)
    {
        if(s.length() < i)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

There's still a double setting of X to s, though. If you wanted to change the characters you'd have to change it in two places. Maybe you should just make the inner loop also do 10 iterations?

for(int i = 0; i < 10; i++)
{
    String s = "";
    for(int j = 0; j < 10; j++)
    {
        if(s.length() <= i)
        {
            s += "X";
        } 
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

Note that I have changed the s.length() < i to s.length() <= i because now the first iteration also needs to put an X.

The result

What we've just done, is step for step, identifying duplication or similarities, and then looking for the differences in that duplication to merge the duplication. The end result is code which, whilst containing more complexity because it doesn't spell out each case individually, is considerably shorter and should be more understandable.

\$\endgroup\$
  • 6
    \$\begingroup\$ Not only is the resulting code more understandable, it's also a direct result of the original code being optimized step-by-step keeping it understandable for the author of the question as well. On top of that, there is no longer an awful amount of needless repetition. Code should never be needless. This answer shows how it's done. \$\endgroup\$ – Mast Sep 2 '16 at 12:03
  • 2
    \$\begingroup\$ Great and detailed job. I just would like to add one remark on the final result: You shouldn't use String concatenation in loops, but rather use StringBuilder because concatenation of immutable Strings is a quite expensive operation, StringBuilder on the other hand is made for that. \$\endgroup\$ – Markus Mitterauer Sep 3 '16 at 10:10
  • \$\begingroup\$ @MarkusMitterauer I figured that was covered in the other answers already. \$\endgroup\$ – Pimgd Sep 3 '16 at 11:34
  • 1
    \$\begingroup\$ I believe you misunderstood the meaning of ft. It looks like it meant "first time in the inner loop". At that time the OP adds all the required X's. The transition from s += "XX"; ft = false; to s += "X"; ft = s.length() < 3; and removing the conditional break is a major change in the algorithm and not a small optimization. And s.length() < 3; should be replaced by j < 3. At that point you could as well rewrite the whole program. But besides that, nice write-up explaining common pitfalls. \$\endgroup\$ – Florian F Sep 3 '16 at 12:14
  • 1
    \$\begingroup\$ This was brilliant. It's always enlightening to see such an structured way of approaching a program refactor. Thanks for a very interesting read. \$\endgroup\$ – carlossierra Sep 3 '16 at 23:16
15
\$\begingroup\$
for (int i = 0; i < 10; i++) {
    System.out.println("XXXXXXXXXXOOOOOOOOO".substring(9 - i, 19 - i));
}

This uses a string of 10 Xs and 9 Os

It suggests that you could do:

for (int i = 0; i < 10; i++) {
    int xs = 1 + i;
    int os = 9 - i;
    // xs + os == 10
    ... call System.out.print("X"); in a loop
    ... call System.out.print("O"); in a loop
    System.out.println();
}

The forgotten review

You asked for a totally different approach, less tedious. So let's give a perspective of some higher abstractness.

Typically (in math for instance) you are distinghuishing cases you handle. So you get if i == 1 and so on. That is a very static view here. In the end you listed all changes possible. However here there is change and you can look for dynamic laws, unvarying conditions (like in physics).

That would be that the number of Xs and Os are and stay 10 and that the number of Xs increase at every i, and the number of Os decrease at every i.

Also you see something happening in two dimensions. Two nested for-loops should be the ordinary intuitive approach (not necessarily right though). The change is minimal line by line, one X/O. It is related to i only. There is just one free variable, freedom, here, so it is a linear problem.

Now back on earth, a valid approach would be:

for i from 0 to 10
    for _ from 0 to i+1 do print "X"
    for _ from i+1 to 10 do print "O"
    newline

or

for i from 0 to 10
    for j from 0 to 10 do print if j <= i then "X" else "O";
    newline

As for less tedious, more playful, that would be "XXX...OOO".substring, seeing a moving substring. Though creative, it hardly minimalistic, maybe in lines of code.

\$\endgroup\$
  • 1
    \$\begingroup\$ That first block of code is really clever, kind of out of the box \$\endgroup\$ – Marv Sep 2 '16 at 8:42
  • 1
    \$\begingroup\$ Congratulations on cleaning-up your answer :-) \$\endgroup\$ – Mast Sep 2 '16 at 12:31
  • \$\begingroup\$ That first block of code is a bit too clever for my taste. If you would use it then you would have to calculate a lot and it's not easy to follow the indexes in a long string with your eyes. \$\endgroup\$ – Simon Forsberg Sep 6 '16 at 16:11
  • \$\begingroup\$ @SimonForsberg yes, with naming, like "rollingText", and other measures. I would not use such too clever code. However here I would almost use it with a comment. Or introduce a String repeat(char ch, int times) or such. \$\endgroup\$ – Joop Eggen Sep 6 '16 at 18:53
11
\$\begingroup\$

You are doing a lot of if-condition to find what you need to print. This works but this is a verbose. You could find some logic in what you want to print.

Your output is a simple square matrix that you want to split by it's diagonal. Here is the matrix :

   0x0   0-1   0-2   0-3   0-4   0-5   0-6   0-7   0-8   0-9
   1-0   1x1   1-2   1-3   1-4   1-5   1-6   1-7   1-8   1-9
   2-0   2-1   2x2   2-3   2-4   2-5   2-6   2-7   2-8   2-9
   3-0   3-1   3-2   3x3   3-4   3-5   3-6   3-7   3-8   3-9
   4-0   4-1   4-2   4-3   4x4   4-5   4-6   4-7   4-8   4-9
   5-0   5-1   5-2   5-3   5-4   5x5   5-6   5-7   5-8   5-9
   6-0   6-1   6-2   6-3   6-4   6-5   6x6   6-7   6-8   6-9
   7-0   7-1   7-2   7-3   7-4   7-5   7-6   7x7   7-8   7-9
   8-0   8-1   8-2   8-3   8-4   8-5   8-6   8-7   8x8   8-9
   9-0   9-1   9-2   9-3   9-4   9-5   9-6   9-7   9-8   9x9

I've highlighted the diagonal by using a x instead of an -.

And here is what you want to print (with the same spaces)

x    o    o    o    o    o    o    o    o    o
x    x    o    o    o    o    o    o    o    o
x    x    x    o    o    o    o    o    o    o
x    x    x    x    o    o    o    o    o    o
x    x    x    x    x    o    o    o    o    o
x    x    x    x    x    x    o    o    o    o
x    x    x    x    x    x    x    o    o    o
x    x    x    x    x    x    x    x    o    o
x    x    x    x    x    x    x    x    x    o
x    x    x    x    x    x    x    x    x    x

You can see some pattern here. Every place you want an ois on the right of the diagonal.

So you can write this as a condition depending on x and y like this :

  • if x < y : we are on the right of the diagonal-> O
  • if not x < y : we are on the left or on the diagonal -> X

You could use a if-else to print x or o but I like to do it on one line so I used use a simple ternary condition: (ternary is simple condition ? true statement : false statement)

       for(int x = 0; x < 10 ; ++x){
            for(int y = 0; y < 10 ; ++y){
                System.out.print(x < y ? 'o' : 'x');
            }
            System.out.println();
        }
\$\endgroup\$
  • \$\begingroup\$ It would make sense to name the loop variables x and y to match your explanation, also your logic in the code is if y <= x - while it's the same as if x > y it requires an additional through process to translate between code and explanation \$\endgroup\$ – ChrisWue Sep 2 '16 at 9:00
  • \$\begingroup\$ Yeah, x and y is more logics since I explained it has a matrix. Will edit it. I used <= to keep the 'x' before the 'o'since we want to put x first (just to keep it simple). I will try to explain a bit better (I am not english native, sorry). \$\endgroup\$ – AxelH Sep 2 '16 at 9:21
  • 2
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – Pimgd Sep 2 '16 at 9:59
  • \$\begingroup\$ Actually, I used his logic of double loops but instead of using a if like his for each value of i, I used both coordinates values. That's why I explain the matrix a used. What would you want to be more explained ? \$\endgroup\$ – AxelH Sep 2 '16 at 10:14
  • 1
    \$\begingroup\$ @AxelH that's a lot better. \$\endgroup\$ – Pimgd Sep 2 '16 at 10:35
7
\$\begingroup\$

Your code is not good, because you have a lot of duplicate lines, you create a lot of strings during the execution of your loop ( because strings are immutable, + creates a new string object). You have a lot of if statements which have to be evaluated and some other things.

An alternative solution is:

public static void main(String[] args) {
    // Algorithm 1
    String o = null;
    for (int i = 0; i < 10; i++) {
        String x = StringUtils.repeat("X", i+1);
        o =StringUtils.repeat("0", 9-i); 
        System.out.println(x+o);

    }
}

StringUtils.repeat is a helper method, which will print a character n times. So I create a string with n times a X and then I do it same with o for the rest and concatenate it for output.

The solution makes your code shorter, more readable, and it uses well tested functions from third party distributor. Why invent the wheel again?

For this solution you need to add the commons-lang3 library to your build/classpath.

\$\endgroup\$
  • \$\begingroup\$ @Jens That's good for explaining your solution, but what I'm missing is how this improves on the original code. I know it may seem tedious to have to explicitly describe the improvements, but imagine OP is really new to Java programming - describe WHY your solution is better so that the OP can judge changes by himself \$\endgroup\$ – Pimgd Sep 2 '16 at 10:07
  • \$\begingroup\$ @Downvoter: be so fair and tell why you downvote \$\endgroup\$ – Jens Sep 2 '16 at 11:29
5
\$\begingroup\$

You have rightfully noted that your code has issues.

To start with, there are problems with the "packaging" of the code. The program doesn't print "Hello World", so Square would be a better name for the class. The class should have a function that accepts the size and the fill characters as parameters.

One problem is that you don't really have an algorithm. The code is completely hard-wired to do one thing only. Changing anything about the output — the size of the square or the fill characters — would require a major overhaul of the entire program. You would already be better off writing this, which is easier to understand, more maintainable, and faster:

System.out.print("XOOOOOOOOO\n" +
                 "XXOOOOOOOO\n" +
                 "XXXOOOOOOO\n" +
                 "XXXXOOOOOO\n" +
                 "XXXXXOOOOO\n" +
                 "XXXXXXOOOO\n" +
                 "XXXXXXXOOO\n" +
                 "XXXXXXXXOO\n" +
                 "XXXXXXXXXO\n" +
                 "XXXXXXXXXX\n");

Repeated string concatenation using += is bad for performance. Strings are immutable, so the += operation constructs a new string, copies the original contents, then copies the portion to be appended. (Usually, a StringBuilder is the recommended remedy.)

In this case, though, even a StringBuilder would be overkill, since every line is the same length. You can just use a char[] array. After you fill it, only one character changes between consecutive lines.

import java.util.Arrays;

public class Square {
    public static void printSquare(int size, char lowerLeft, char upperRight) {
        char[] line = new char[size];
        Arrays.fill(line, upperRight);
        for (int i = 0; i < size; i++) {
            line[i] = lowerLeft;
            System.out.println(new String(line));
        }
    }

    public static void main(String[] args) {
        printSquare(10, 'X', 'O');
    }
}
\$\endgroup\$
3
\$\begingroup\$

Each line of your output is the same as the previous but with one character different. Thus a StringBuilder can be used to hold the string such that it can be manipulated. The StringBuilder can be converted into a String at any point for display purposes.

StringBuilder sb = new StringBuilder("OOOOOOOOOO");
for (int i = 0; i < 10; i++)
{
    sb.setCharAt(i, 'X');
    System.out.println(sb);
}
\$\endgroup\$
2
\$\begingroup\$

As you can see, your code works but is long-winded and confusing to read.

There are many solutions to this problem. You could refine your loops and if statements, which would make the code easier to read.

Alternatively, you could take a different logical approach. Each line of output can be split into two strings of repeated characters, with varying lengths.

You can create those two strings of characters in a single statement. They can then be concatenated together and printed:

    for(int i = 0; i < 10; i++)
      System.out.println(new String(new char[i+1]).replace("\0", "X") + new String(new char[9-i]).replace("\0", "O"));

For each iteration of the for loop, this creates two new strings of length i+1 and 9-i, filling them with NULL characters, it then replaces the characters in the first with 'X' and the second with 'O'.

\$\endgroup\$

protected by Mathieu Guindon Sep 2 '16 at 13:34

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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