3
\$\begingroup\$

I'm working with parameters right now in Java, and my colleague told me that this code of mine is redundant, and I see his point. The for loops do look redundant, but I have no idea how to go about changing this. My code is functional, but I wish to improve it.

public static void printLine(int length) 
{
    for (int i = 1; i <= length; i++) 
    {
        System.out.print("*");
    }       
    System.out.println();
}

public static void printBox(int length, int height) 
{
    printLine(length);

    for (int i = 1; i <= height - 2; i++) 
    {
    System.out.print("*");     
        for (int j = 1; j <= length - 2; j++) 
        {
            System.out.print(" ");
        }
        System.out.println("*");
    } 
    printLine(length);     
} 
\$\endgroup\$
5
\$\begingroup\$

Your friend is right, and there's ways to parameterize the code to make it more useful. Consider three things:

  1. Don't print each character - build a block of text up, and print the block (note that adding a newline to the string \n works as well as a System.out.println())
  2. You can input the text to repeat as a parameter to the function...
  3. It is common practice to have loops from 0 to 1-less-than-the-limit like for (int i = 0; i < limit; i++) {....} and not from 1 to the limit for (int i = 1; i <= limit; i++) {...}. Your loop is not wrong.... but it is unconventional.

So, take your function:

public static void printLine(int length) 
{
    for (int i = 1; i <= length; i++) 
    {
        System.out.print("*");
    }       
    System.out.println();
}

And apply the three concepts above... and we get:

public static String repeatText(String text, int count) {
    StringBuilder output = new StringBuilder();
    for (int i = 0; i < count; i++) {
        output.append(text);
    }       
    return output.toString();
}

Now, that function can be used as the basis for a bunch of things....

  1. your top-and-bottom lines of the box.
  2. the inside-content of the other lines of the box
  3. the repeating rows inside the box

Consider your printBox function:

public static void printBox(int length, int height) 
{
    printLine(length);

    for (int i = 1; i <= height - 2; i++) 
    {
    System.out.print("*");     
        for (int j = 1; j <= length - 2; j++) 
        {
            System.out.print(" ");
        }
        System.out.println("*");
    } 
    printLine(length);     
}

Let's use the same 3 principles I listed earlier, but let's also use the new repeatText function....

public static String buildBox(int length, int height)  {
    String topAndBottom = repeatText("*", length) + "\n";
    String middle = "*" + repeatText(" ", length - 2) + "*\n";
    String box = topAndBottom + repeatText(middle, height - 2) + topAndBottom;
    return box;
}

Now, you can print that box just once with:

System.out.print(buildBox(length, height));

There are still issues, like what happens if the length or height are less than 2? That would be a problem. Consider ways to avoid that, or handle that.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would note that your implementation has exactly the same loops as OP, because the loops are not redundant. Your improvement comes from using a string builder rather than string concatenation. You also convert back and forth between StringBuilder more than is necesarry, IMHO repeatText should take a StringBuilder as argument, rather than return a string. Also OP's code might actually be better if length and height gets very large. In that case producing each line seperately has obvious advantages over constructing the complete string. \$\endgroup\$ – Taemyr Oct 4 '16 at 8:46
  • \$\begingroup\$ Umm, just realised. You do eliminate looping, since you only do the inner loop once. \$\endgroup\$ – Taemyr Oct 4 '16 at 9:14
  • \$\begingroup\$ @Taemyr - interesting thoughts, but the OP is obviously a novice, and I kept my suggestions contained to just a few items. The code redundancy is more important than the loop iterations. I did not use "clean code" names but essentially I recommended SRP, functional extractions, common-practice, and not much more. Even what i have may be more than a novice can absorb in one review. Your suggestions, in my opinion, go too far in to issues that are secondary to where the OP in the project he is working in right now. Put together your own answer if you think I missed important suggestions. \$\endgroup\$ – rolfl Oct 4 '16 at 10:40
3
\$\begingroup\$

Your solution doesn't handle the degenerate case of a box whose height is less than or equal to 1.

I suggest renaming length to width for clarity.

It would probably be good practice to make your printLine() helper function private.

Your solution actually isn't that bad in terms of repetitiveness. However, since each call to System.out.print() has significant overhead, it would be more efficient if you could print out larger chunks of output at a time.

I suggest making use of java.util.Arrays.fill() to create two strings, then reuse them.

import java.util.Arrays;

public class Box {
    private static String fill(char[] buf, char edge, char mid) {
        Arrays.fill(buf, mid);
        if (buf.length > 0) buf[0] = buf[buf.length - 1] = edge;
        return new String(buf);
    }

    public static void printBox(int width, int height) {
        char[] line = new char[width];
        String topBottom = fill(line, '*', '*');
        String body = fill(line, '*', ' ');

        if (height >= 1) System.out.println(topBottom);
        for (int i = 1; i < height - 1; i++) {
            System.out.println(body);
        }
        if (height >= 2) System.out.println(topBottom);
    }
}
\$\endgroup\$

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.