Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have written this code that takes input from a file and creates an output file that reports each person's name, percentage of B answers, and personality. However, the my computePersonality method, which takes the contents of an integer array and converts it into a string of \$4\$ letters based on if the number in the array is greater than, equal to, or less than \$50\$, is pretty long.

I'm worried about the length of the method and the redundancy? Also, any comments on the efficiency of the overall code would be appreciated.

Here's an example of an input file:

Betty Boop
BABAAAABAAAAAAABAAAABBAAAAAABAAAABABAABAAABABABAABAAAAAABAAAAAABAAAAAA
Snoopy
AABBAABBBBBABABAAAAABABBAABBAAAABBBAAABAABAABABAAAABAABBBBAAABBAABABBB
Bugs Bunny
aabaabbabbbaaaabaaaabaaaaababbbaabaaaabaabbbbabaaaabaabaaaaaabbaaaaabb
Daffy Duck
BAAAAA-BAAAABABAAAAAABA-AAAABABAAAABAABAA-BAAABAABAAAAAABA-BAAABA-BAAA

My main concern is the computePersonality. Here's the complete class:

import java.util.*;
import java.io.*;

public class Personality {

   public static final int CONSTANT = 4;

   public static void main(String[] args) throws FileNotFoundException {
      Scanner console = new Scanner(System.in);
      giveIntro();
      System.out.print("input file name? ");
      String inputFile = console.next();
      System.out.print("output file name? ");
      String outputFile = console.next();
      Scanner input = new Scanner(new File(inputFile));
      PrintStream output = new PrintStream(new File(outputFile));
      processFile(input, output);      

   }

   public static void giveIntro() {
      System.out.println("This program processes a file of answers to the");
      System.out.println("Keirsey Temperament Sorter.  It converts the");
      System.out.println("various A and B answers for each person into");
      System.out.println("a sequence of B-percentages and then into a");
      System.out.println("four-letter personality type.");
      System.out.println();   
   }

   public static void processFile(Scanner input, PrintStream output) {
      while (input.hasNextLine()) {
         String name = input.nextLine();
         String data = input.nextLine();
         int[] totalA = new int[7];
         int[] totalB = new int[7];
         int[] totalAB = new int[CONSTANT];
         computeScores(data, totalA, totalB);
         int[] countsA = new int[CONSTANT];
         int[] countsB = new int[CONSTANT];
         int[] percentB = new int[CONSTANT];
         countsOfAB(totalA, totalB, countsA, countsB, totalAB);
         computePercentB(countsB, totalAB, percentB);
         String personality = computePersonality(percentB);
         print(output, name, percentB, personality);

      }   
   }

   public static void computeScores(String data, int[] totalA, int[] totalB) {
      for (int i = 0; i < 7; i++) {
         for (int j = i; j < data.length(); j += 7) {
            char c = data.charAt(j);
            if (c == 'a' || c == 'A') {
               totalA[i]++;
            } else if (c == 'b' || c == 'B') {
               totalB[i]++;         
            }
         }
      }                    
   }

   public static void countsOfAB(int[] totalA, int[] totalB, int[] countsA, int[] countsB, int[] totalAB) {
      countsA[0] = totalA[0];
      countsB[0] = totalB[0];
      totalAB[0] = totalA[0] + totalB[0];
      for (int j = 2; j < 7; j += 2) {   
         for (int i = j / 2; i < CONSTANT; i++) {
            countsA[i] = totalA[j] + totalA[j - 1];
            countsB[i] = totalB[j] + totalB[j - 1];
            totalAB[i] = countsA[i] + countsB[i];
         }
      }                 
   }

   public static void computePercentB(int[] countsB, int[] totalAB, int[]   percentB) {
      for (int i = 0; i < CONSTANT; i++) {
         percentB[i] = (int) Math.round(countsB[i] / (double) totalAB[i] * 100) ;
      }                 
   }

   public static String computePersonality(int[] percentB) {
      String personality = "";
      if (percentB[0] < 50) {
         personality += "E";
      } else if (percentB[0] > 50) {
         personality += "I";
      } else { 
         personality += "X";
      }   
      if (percentB[1] < 50) {
         personality += "S";
      } else if (percentB[1] > 50) {
         personality += "N";
      } else {
         personality += "X";
      }   
      if (percentB[2] < 50) {
         personality += "T";
      } else if (percentB[2] > 50) {
         personality += "F";
      } else {
         personality += "X";
      }
      if (percentB[3] < 50) {
         personality += "J";
      } else if (percentB[3] > 50) {
         personality += "P";
      } else {
         personality += "X";
      }
      return personality;
   }

   public static void print(PrintStream output, String name, int[] percentB, String personality) {
      String percent = Arrays.toString(percentB);
      output.println(name + ": " + percent + " = " + personality);  
   }                                               
}
share|improve this question
    
Nice first question, an example input file will make it easier to review the code and allow you to get even better reviews :) – Caridorc yesterday
    
Thank you for the tip! – t.h.h yesterday
    
Welcome :) we always like when users care to ask good question (run-snippet only works with Javascript) – Caridorc yesterday
    
Are you on Java 8? – h.j.k. yesterday
    
BTW, do you have sample output as well? – h.j.k. 19 hours ago

2 Answers 2

Sorry it's getting late now, still I want to give you my first suggestions.

  • Try to choose more meaningful names for variables (including constants). Example: CONSTANT -> NUMBER_OF_PERSONALITY_TYPES

  • I generalized your computePersonality function (untested, care!).

  • Some line breaks always improve readability in my opinion.

Next steps:

  • Next I would extract the block of code inside the while loop within the processFile method. This is now the longest method, maybe we can break it down some more.

  • Extract the remaining constants (at least the number 7 as far as I have seen) and give it meaningful names.

import java.io.File;
import java.io.FileNotFoundException;
import java.io.PrintStream;
import java.util.Arrays;
import java.util.Scanner;

public class Personality {

    public static final int NUMBER_OF_PERSONALITY_TYPES = 4;

    // this is not an optimal name
    public static final int PERCENT_B_MIDDLE = 50;

    public static void main(String[] args) throws FileNotFoundException {

        Scanner console = new Scanner(System.in);
        giveIntro();

        System.out.print("input file name? ");
        String inputFile = console.next();

        System.out.print("output file name? ");
        String outputFile = console.next();

        Scanner input = new Scanner(new File(inputFile));
        PrintStream output = new PrintStream(new File(outputFile));

        processFile(input, output);
        console.close();

    }

    public static void giveIntro() {

        System.out.println("This program processes a file of answers to the");
        System.out.println("Keirsey Temperament Sorter.  It converts the");
        System.out.println("various A and B answers for each person into");
        System.out.println("a sequence of B-percentages and then into a");
        System.out.println("four-letter personality type.");
        System.out.println();
    }

    public static void processFile(Scanner input, PrintStream output) {

        while (input.hasNextLine()) {

            String name = input.nextLine();
            String data = input.nextLine();
            int[] totalA = new int[7];
            int[] totalB = new int[7];
            int[] totalAB = new int[NUMBER_OF_PERSONALITY_TYPES];
            computeScores(data, totalA, totalB);
            int[] countsA = new int[NUMBER_OF_PERSONALITY_TYPES];
            int[] countsB = new int[NUMBER_OF_PERSONALITY_TYPES];
            int[] percentB = new int[NUMBER_OF_PERSONALITY_TYPES];
            countsOfAB(totalA, totalB, countsA, countsB, totalAB);
            computePercentB(countsB, totalAB, percentB);
            String personality = computePersonality(percentB);
            print(output, name, percentB, personality);

        }
    }

    public static void computeScores(String data, int[] totalA, int[] totalB) {

        for (int i = 0; i < 7; i++) {
            for (int j = i; j < data.length(); j += 7) {
                char c = data.charAt(j);
                if (c == 'a' || c == 'A') {
                    totalA[i]++;
                } else if (c == 'b' || c == 'B') {
                    totalB[i]++;
                }
            }
        }
    }

    public static void countsOfAB(int[] totalA, int[] totalB, int[] countsA, int[] countsB, int[] totalAB) {

        countsA[0] = totalA[0];
        countsB[0] = totalB[0];
        totalAB[0] = totalA[0] + totalB[0];

        for (int j = 2; j < 7; j += 2) {

            for (int i = j / 2; i < NUMBER_OF_PERSONALITY_TYPES; i++) {

                countsA[i] = totalA[j] + totalA[j - 1];
                countsB[i] = totalB[j] + totalB[j - 1];
                totalAB[i] = countsA[i] + countsB[i];
            }
        }
    }

    public static void computePercentB(int[] countsB, int[] totalAB, int[] percentB) {

        for (int i = 0; i < NUMBER_OF_PERSONALITY_TYPES; i++) {

            percentB[i] = (int) Math.round(countsB[i] / (double) totalAB[i] * 100);
        }
    }

    public static String computePersonality(int[] percentB) {

        StringBuilder personality = new StringBuilder();

        personality.append(getLetterByScore(percentB[0], "E", "I", "X"));
        personality.append(getLetterByScore(percentB[1], "S", "N", "X"));
        personality.append(getLetterByScore(percentB[2], "T", "F", "X"));
        personality.append(getLetterByScore(percentB[3], "J", "P", "X"));

        return personality.toString();
    }

    private static String getLetterByScore(int percentBScore, String letterOne, String letterTwo, String letterThree) {

        if (percentBScore < PERCENT_B_MIDDLE) {

            return letterOne;

        } else if (percentBScore > PERCENT_B_MIDDLE) {

            return letterTwo;

        } else {

            return letterThree;
        }
    }

    public static void print(PrintStream output, String name, int[] percentB, String personality) {

        String percent = Arrays.toString(percentB);
        output.println(name + ": " + percent + " = " + personality);
    }
}
share|improve this answer

computePersonality

Your main concern. There are too many repetitive if-elseif-else. The personality type letters can be extracted into a constant matrix, for example:

private static final char[][] PERSONALITY_MATRIX = {
  { 'E', 'I' },
  { 'S', 'N' },
  { 'T', 'F' },
  { 'J', 'P' },
};

private static int SCORE_THRESHOLD = 50;

The letter 'X' is not there, because it is common for all cases. Don't need to repeat it in the matrix.

Repetitive String concatenation is also an evil, even though in your example it's just single characters. Use StringBuilder for such cases.

And the body of the method becomes something like:

public static String computePersonality2(int[] percentB) {
  final StringBuilder bld = new StringBuilder();
  for (int i = 0; i < PERSONALITY_MATRIX.length; i++) {
    final int score = percentB[i];
    char mark = 'X';
    if (score < SCORE_THRESHOLD) {
      mark = PERSONALITY_MATRIX[i][0];
    } else if (score > SCORE_THRESHOLD) {
      mark = PERSONALITY_MATRIX[i][1];
    }
    bld.append(mark);
  }
  return bld.toString();

}

I/O

None of Scanner or PrintStream resources seem to be closed. There should be a try-with-resources block in the main method or at least .close() calls.

Naming and Magic Numbers

What does the int CONSTANT mean? Is this the definitive constant of the Universe? Another name for it, more meaningful, would be better.

Just before the lines with CONSTANT, there are occurrences of '7', which may be also seen in next methods. This number should be extracted into another constant.

Puzzles?

computeScores and countsOfAB contain some sort of puzzle-like nested for loops, which may be rather difficult to debug and understand what exactly they count through the iterations.

Passing parameters by reference, like it is done for total* and count* arrays, is neither a very good practice. It is preferable to return values from a method. If there are several values to return at the same time, a dedicated object definition would help to solve the issue.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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