Using contents of an integer array to print a String

Question

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); 
 } 
}

• h.j.k.: Are you on Java 8?
• h.j.k.: BTW, do you have sample output as well?
• h.j.k.: I have posted a rags-to-riches question here with an improved solution to my answer here, do check it out too.

Answer 1

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);
 }
}

Answer 2

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.

Answer 3

Returning results from methods

Most of your processing methods except for computePersonality() mutate the method arguments as a way of passing the results back to the method caller. There is nothing inherently wrong with this approach, but it does mean you have to be careful with the ordering of your method arguments (since your results are just int[] arrays), and you will not be able to chain method calls together.

In some cases, it might be better to use a payload class, even if it's a custom one, to better model a result from a method. For example, instead of passing around two int[] arrays, you can have a Result class that provide methods like updateA(int[]) or computePercentage() to better encapsulate such functionality.

Enumerating and validating inputs

Since your processing is only done on A or B (case-insensitive) values, an enum may be a suitable option of representing these choices:

enum Choice {
 A, B, UNKNOWN;
 public static Choice of(char x) {
 try {
 return Choice.valueOf(String.valueOf(x).toUpperCase());
 } catch (IllegalArgumentException e) {
 return Choice.UNKNOWN;
 }
 }
}

In this case, we 'convert' "A"/"a" as Choice.A, and "B"/"b" as Choice.B. Other values are simply represented as Choice.UNKNOWN.

Computing and grouping scores

edit: I have posted a rags-to-riches question here with an improved solution, do check it out too.

Based on my interpretation of your countsOfAB() method, the computation seems like:

1. Divide the 70 responses into 10 chunks of 7 responses each:

   ABABABaABABABaABABABa...ABABABa
   <- Q -><- R -><- S ->...<- Z ->
   ABABABa: chunk Q
   ABABABa: chunk R
   ABABABa: chunk S
   ...
   ABABABa: chunk Z
   

2. From each chunk, the 1st response goes into its own grouping (i.e. the \0ドル^{th}\$ element of your int[] countsA/B array), the 2nd and 3rd responses goes into the next, and finally the last two into the 4th grouping:

$$ A \rbrace 0 \\ \left.\begin{matrix} B \\ A \\ \end{matrix}\right\rbrace 1 \\ \left.\begin{matrix} B \\ A \\ \end{matrix}\right\rbrace 2 \\ \left.\begin{matrix} B \\ a \\ \end{matrix}\right\rbrace 3 $$

3. Collate all the groupings across the 10 chunks, so that for the example, we have the following representations:

$$\begin{array}{|c|c|c|} \hline Grouping & Count for A & Count for B\\ \hline 0 & 10 & 0 \\ \hline 1 & 10 & 10 \\ \hline 2 & 10 & 10 \\ \hline 3 & 10 & 10 \\ \hline \end{array} $$

With a bit of math, it is not hard to combine these into a single step, especially when you involve Java 8's stream:

private static final int GROUPING = 7;
private static Map<Choice, Map<Integer, Long>> compute(String input) {
 return join(IntStream.range(0, input.length()).mapToObj(i -> map(input, i)))
 .collect(Collectors.groupingBy(Entry::getKey,
 Collectors.groupingBy(Entry::getValue, Collectors.counting())));
}
private static Map<Choice, Integer> map(String input, int i) {
 return Collections.singletonMap(Choice.of(input.charAt(i)), ((i % GROUPING) + 1) / 2);
}
private static <K, V> Stream<Entry<K, V>> join(Stream<Map<K, V>> stream) {
 return stream.map(Map::entrySet).flatMap(Set::stream);
}

join() is a simple utility method that converts a Stream of Map elements into a Stream of their Entry contents.

The magic lies in the map(String, int) method, which creates a Choice \$\to\$ grouping mapping by the formula ((i % GROUPING) + 1) / 2 (which is implicitly truncated to an int):

$$\begin{array}{|c|c|} \hline i & f(i)\\ \hline 0 & 0 \\ \hline 1 & 1 \\ \hline 2 & 1 \\ \hline 3 & 2 \\ \hline 4 & 2 \\ \hline 5 & 3 \\ \hline 6 & 3 \\ \hline 7 & 0 \\ \hline 8 & 1 \\ \hline 9 & 1 \\ \hline \cdots & \cdots \\ \hline 67 & 2 \\ \hline 68 & 3 \\ \hline 69 & 3 \\ \hline \end{array} $$

In the compute() method, we create these mappings per response, join() them together, and then return the desired Map<Choice, Map<Integer, Long>> object groupingBy() the Choice key (using Entry::getKey as a method reference), and counting() the values (Entry::getValue) after groupingBy() them as well.

The Map representation for the example is thus:

{B={1=10, 2=10, 3=10}, A={0=10, 1=10, 2=10, 3=10}}

There is no 0=... result for choice B, since all the responses in that grouping are A.

Calculating the scores and mapping

Next, we need to calculate the percentages of B responses per group, i.e. we will have 0% for group 0 and 50% for the rest for the example.

From the Map<Choice, Map<Integer, Long>> result we have previously, it is again not hard to apply Java 8's stream processing again to perform the calculation and do the mapping.

Let's introduce some constants first:

private static final String[][] TRAITS = Stream.of("EXI", "SXN", "TXF", "JXP")
 .map(v -> v.split(""))
 .toArray(String[][]::new);
private static final Map<Integer, Long> BLANK =
 Collections.unmodifiableMap(IntStream.range(0, TRAITS.length).boxed()
 .collect(Collectors.toMap(i -> i, i -> 0L)));
private static final int THRESHOLD = 50;

TRAITS is a similar take on the array-based lookup approach suggested in @Antot's answer, except that it is String[][]-based and contains the "X" values for easier lookups when the percentage is exactly 50%, i.e. THRESHOLD. BLANK represents a Map where there are no responses for a given choice.

public static String getPersonality(String input) {
 Map<Choice, Map<Integer, Long>> map = compute(Objects.requireNonNull(input));
 return join(Stream.of(Choice.A, Choice.B).map(v -> map.getOrDefault(v, BLANK)))
 .collect(Collectors.groupingBy(Entry::getKey,
 Collectors.summingDouble(Entry::getValue)))
 .entrySet()
 .stream()
 .map(entry -> derive(map.getOrDefault(Choice.B, BLANK), entry))
 .collect(Collectors.joining());
}
private static String derive(Map<Integer, Long> m, Entry<Integer, Double> i) {
 return TRAITS[i.getKey()][index(m.getOrDefault(i.getKey(), 0L), i.getValue())];
}
private static int index(long numerator, double denominator) {
 int value = (int) Math.round(100 * numerator / denominator);
 return value < THRESHOLD ? 0 : value == THRESHOLD ? 1 : 2;
}

Since we are only interested in A and B responses, we query for the map results via Stream.of(Choice.A, Choice.B).map(v -> map.getOrDefault(v, BLANK)). Using BLANK as a default value here is crucial to provide 0s for the calculations later.

At the end of the first collect() step, what we have is a Map<Integer, Double> representing the summation of responses per grouping, by applying groupingBy() on the key and summingDouble() on the values.

For each of the entries (groupings \0ドル\ldots3\$), we can now derive the personality trait with the method derive(). Its first argument is the results of B responses from the compute() method, and the second argument is the Map entry of grouping \$\to\$ total responses of the grouping.

From this entry, the key (grouping) is used for the first dimension of our TRAITS array, and the second dimension is the result of computing our 'B-percentage' in the index() method.

Finally, each resulting String from the derive() method is joined together to give the four-letter personality by using Collectors.joining().

Conclusion

This is quite a lengthy answer, but I hope it adequately describes how the various steps in your solution can be re-imagined and re-composed into what are essentially two stream-based operations (helper methods aside).