2
\$\begingroup\$

I have created a Mitsubishi logo builder that takes input from the logo.txt file and uses those ints/chars to create however many logos that are identified in the logo.txt file.

I just feel that my code is very repetitive and I would like to clean it up in terms of cutting it into separate methods instead of repeating over and over. Are there any recommendations on how to clean this up? I would appreciate any and all feedback. Or even just a starting point.

public static void main(String[] args) {

    Scanner input = new Scanner(System.in);

    try {
        input = new Scanner(new FileInputStream("config.txt"));
        //initialize ithe variable's input and output
    } catch (FileNotFoundException e) {
        System.out.println("File not found.");
        System.exit(0);

    }

    int size = Integer.parseInt(inputStream.nextLine());
    int numLogos = Integer.parseInt(inputStream.nextLine());
    char letter1 =  inputStream.next().charAt(0);
    char letter2 =  inputStream.next().charAt(0); 

    for(int bee = numLogos; bee > 0; bee--){ 
        for(int y = 0; y < size; y++) { 

            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }

            for(int x = size; x > y; x--) {
                System.out.print(letter2);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter1);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter1);
            }
            for(int x = size; x > y; x--) {
                System.out.print(letter2);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }
            System.out.println();
        }           
        for(int y = 0; y < size; y++) {
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }            
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter1);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter1);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter2);
            }
            System.out.println();
        }
        for(int y = 0; y < size; y++) {
            for(int x = size; x > y; x--) {
                System.out.print(letter2);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter1);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter1);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter1);
            }

            for(int x = size; x > y; x--) {
                System.out.print(letter1);
            }
            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter2);
            }            
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter1);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter1);
            }
            for(int x = 0; x < size - y; x++) {
                System.out.print(letter1);
            }

            for(int x = 0; x < y; x++) {
                System.out.print(letter1);
            }
            for(int x = size; x > y; x--) {
                System.out.print(letter2);
            }
            System.out.println();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Probably you need to replace inputStream with input \$\endgroup\$ – default locale Feb 15 '17 at 11:24
4
\$\begingroup\$

You can reduce duplication in your code step-by-step by moving repetitive code into separate methods.

1. Move the code that prints one logo into a separate method:

Introduce new method that prints exactly one logo of a given size and then call it in a loop

for(int bee = numLogos; bee > 0; bee--) { 
    printLogo(size);
}

By the way, increasing loop counters are easier to read most of the time. Use them whenever possible:

for(int bee = 0; bee < numLogos; bee++) { 
    printLogo(size);
}

I would also suggest either renaming bee to something meaningful (logoIndex) or replacing it with generic counter variable (i).

2. Merge loops that print the same letter

Consecutive loops that print the same letter, like these:

for(int x = 0; x < y; x++) {
    System.out.print(letter2);
}
for(int x = 0; x < size - y; x++) {
    System.out.print(letter2);
}

, are redundant. You need to merge them into one:

for(int x = 0; x < size; x++) {
    System.out.print(letter2);
}

3. Introduce a method(s) for letter printing

Currently, your code uses two types of loops for printing:

//type 1: prints a given number of letters
for(int x = 0; x < someLimit; x++) {
     System.out.print(someLetter);
}

//type 2: prints a number of letters necessary to leave space at the end
for(int x = size; x > numbersAtTheEnd; x--) {
    System.out.print(someLetter);
}

You can create two separate methods for these operations:

//Example for the first type of loop
public static void printLetter(char letter, int count) {
    for(int x = 0; x < count; x++) {
        System.out.print(letter);
    }
}

public static void printLetterUntil(char letter, int size, int left) {
    for(int x = size; x > left; x--) {
        System.out.print(letter);
    }
}

//then call this method repeatedly:
printLetter(letter2, y);
printLetter(letter2, size-y);
printLetter(letter2, y);

I'd suggest transforming the second type to the first one. It seems that printLetterUntil(letter, size, left) is equivalent to printLetter(letter, size-left), but you'll need to check my logic for off-by-one errors.

4. Figure out a pattern

After previous steps have been completed your main printing method will look approximately like this:

static void printLogo(int size) {
    for(int y = 0; y < size; y++) { 
        printLetter(letter2, someCount);
        printLetter(letter1, anotherCount);
        printLetter(letter2, yetAnotherCount);
        //skipped similar lines
        System.out.println();
    }
    for(int y = 0; y < size; y++) { 
        //second part
        printLetter(letter2, someCount);
        printLetter(letter1, anotherCount);
        //...
    }
    for(int y = 0; y < size; y++) { 
        //third part 
        printLetter(letter2, someCount);
        printLetter(letter1, anotherCount);
        //...
    }
}

These loop bodies look terribly similar to each other. All of them print alternating sequences of letter2 and letter1. To get rid of them you can introduce a method that takes an array of numbers and prints alternating sequences of letters:

 public static void printAlternatingSequence(char letter1, char letter2, int... segments) {
      for(int i=0; i<segments.length; i++) {
           \\print letter2 for even indices and letter1 for odd
           char currentLetter = i%2==0 ? letter2 : letter1;
           printLetter(currentLetter, segments);
      }
 }

Notice the use of varargs parameter. This way you don't have to explicitly create an array.

You can call this method like this:

static void printLogo(int size) {
    for(int y = 0; y < size; y++) { 
        printAlternatingSequence(letter1, letter2, someCount, anotherCount, ...);
    }
    for(int y = 0; y < size; y++) { 
        printAlternatingSequence(letter1, letter2, someCount, anotherCount, ...);
    }
    for(int y = 0; y < size; y++) { 
        printAlternatingSequence(letter1, letter2, someCount, anotherCount, ...);
    }
}

5. Turn parameters into fields.

Notice that some values are used in several methods (letter1, letter2, size). Instead of passing them as an argument in every method you can introduce a new class, declare fields for these values and pass them in a constructor.

Example:

public class MitsubishiLogoPrinter {
    private final int size;
    private final char letter1;
    private final char letter2;

    public MitsubishiLogoPrinter(int size, char letter1, char letter2) {
        this.size = size;
        this.letter1 = letter1;
        this.letter2 = letter2;
    }

    public void printLogo() {
        //skipped implementation
    }

    private void printAlternatingSequence(int... segments) {
        //skipped implementation
    }
}

Further improvements.

Take a look at the lengths of segments printed. Probably, there is a usable pattern there as well. If you can represent the length of each segment as f(size, lineNumber) = k*size+l*lineNumber then you can store quotients in a constant array and main printing method will just loop through it.

\$\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.