Rewriting nested for loops to give better formatted output

Question

The following is code from a university practical I am doing. It reads in a txt file of twenty clients, whose information is stored in the txt file like this:

Sophia Candappa     F 23 00011  

As per my lecturer's instructions, I have stored this information in a class called Client (although I know an ArrayList would be better I can't use it).
The code below is a method that is used to compare all the clients to one another and determine if they are a match. They are a match if they are all of the following:

1. Opposite sex
2. Age within five years of one another
3. They have three interests in common

The latter is determined by the string "00011" in the example above. If the clients share the number "1" at the same place in the string on three or more occasions, then the third condition is satisfied.

My code works perfectly and outputs the desired result. However, I want to ask two questions.

• Is it as efficient as it could be (without ArrayLists)? I had considered separating out all the if/else statements into separate methods, but decided against it as I thought it wouldn't reduce any of the actual loops.

• How can I change the output slightly. Currently, if a client is matched it displays "[Client Name] is compatible with" then it takes a new line and outputs all the clients who are matched. I would like to change it so that if the client has just one match, it says "Client Name is compatible with"..., but if the client has two or more clients it says "Client Name is compatible with the following [two/three/four] clients...

I have tried doing the latter, but I always mess up the formatting. Thanks in advance for any help that can be offered.

public static void matchClients(Client[] clientDetails)
{
    boolean anyMatch;
    int count;
    for (int b = 0; b < numberOfClients; b++)
    {
        anyMatch = false;
        count = 0;
        for (int c = 0; c < numberOfClients; c++)
        {
            if (clientDetails[b].getClientGender()!=clientDetails[c].getClientGender())
            {
                if (Math.abs(clientDetails[b].getClientAge() - clientDetails[c].getClientAge()) <= 5)
                {
                    int interests = 0;

                    String clientOneInterests = clientDetails[b].getClientInterests();
                    String clientTwoInterests = clientDetails[c].getClientInterests();

                    int interestNumber = 0;
                    while (interestNumber < clientOneInterests.length())
                    {
                        if ((clientOneInterests.charAt(interestNumber) == clientTwoInterests.charAt(interestNumber))
                                && (clientOneInterests.charAt(interestNumber) == '1' ))
                            interests++;
                        interestNumber++;
                    }

                    if (interests >= 3)
                    {
                        anyMatch = true;
                        if (count == 0)
                        {
                            System.out.println(clientDetails[b].getClientName() + "is compatible with the following client(s)");
                            System.out.println("\t" + clientDetails[c].getClientName());
                        }
                        else
                        {
                            System.out.println("\t" + clientDetails[c].getClientName());
                        }
                        count++;
                    }
                    interests = 0;
                }
            }
        }
        if (anyMatch == false)
            System.out.println(clientDetails[b].getClientName() + "is not compatible with any client.");
        System.out.println("");
    }
}

Answer 1

1. You should extract out at least two methods:

   boolean isMatchingClients(final Client client, final Client otherClient) { ... }
   int calculateCommonInterests(final String clientInterests, 
       final String otherClientInterests) { ... }
   

   It would improve readabilily a lot and make the code more flatten.

   I usually prefer final variables. Making a variable final relieves the programmer of excess mental juggling - he/she doesn't have to scan through the code to see if the variable has changed. (From @nerdytenor's answers.)

2. Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used.

   For example, the count variable could be right before the second for loop:

   int count = 0;
   for (int c = 0; c < numberOfClients; c++)
   

   (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

3. If it's possible I'd consider storing the interest data in BitSets. Then it would be possible to and two of them and get the number of common bits with the cardinality method. Here is some test code:

   @Test
   public void testBitSet() {
       final BitSet interestBitSet = createBitSetFromString("11100111");
       final BitSet otherInterestBitSet = createBitSetFromString("11001001");
   
       final BitSet clone = (BitSet) interestBitSet.clone();
       clone.and(otherInterestBitSet);
   
       assertEquals(3, clone.cardinality());
   }
   
   private BitSet createBitSetFromString(final String input) {
       final BitSet result = new BitSet();
   
       final char[] inputArray = input.toCharArray();
       for (int index = 0; index < inputArray.length; index++) {
           if (inputArray[index] == '1') {
               result.set(index);
           }
       }
       return result;
   }
   

   The and method modifies the bitset which is a disadvantage here. Cloning helps, although it's not the most beautiful solution.

4. Instead of

   System.out.println("");
   

   you could use

   System.out.println();
   

5. You could remove some duplication:

   if (count == 0) {
       System.out.println(clientDetails[b].getClientName() + "is compatible with the following client(s)");
       System.out.println("\t" + clientDetails[c].getClientName());
   } else {
       System.out.println("\t" + clientDetails[c].getClientName());
   }
   

   The following is the same:

   if (count == 0) {
       System.out.println(clientDetails[b].getClientName() + "is compatible with the following client(s)");
   }
   System.out.println("\t" + clientDetails[c].getClientName());
   

6. I'd create local variables for clientDetails[b] and clientDetails[c]. They are used a lot.

7. About the output: if you want to show the number of compatible clients in the header you have to count them (and maybe store them in an ArrayList to avoid running the loop twice) before printing the header. If you are not allowed to use ArrayLists I don't think that it's worth it.

8. I don't know what's the type of the clientGender field. If it's String, you should not compare it with != or ==. See: Java String.equals versus ==

9. The anyMatch variable is duplicated, it could be eliminated. It only stores the state whether count == 0 or not.

   final boolean anyMatch = (count > 0);
   if (anyMatch) { 
       ... 
   }
   

Some optimization ideas:

1. Use two arrays: one for males, one for females, so you don't have to compare clients with the same sex.

2. Order the arrays by age and skip the definitely non-matching clients. You could use binary search here.

3. Use a two-dimensional array with the first dimension as the age. The second dimension contains the clients with the corresponding age.

4. You could use one array which is sorted by gender then age. You could use Arrays.sort and a custom compareTo method in the Client class:

   @Override
   public int compareTo(final Client o) {
       final int nameResult = name.compareTo(o.name);
       if (nameResult != 0) {
           return nameResult;
       }
       return age - o.age;
   }
   

Finally, here is my two methods after the refactorings mentioned above:

public static void matchClients(final Client[] allClients) {
    for (final Client client: allClients) {
        checkClient(client, allClients);
    }
}

private static void checkClient(final Client client, final Client[] allClients) {
    int matchCount = 0;
    for (final Client otherClient: allClients) {
        if (!isMatchingClients(client, otherClient)) {
            continue;
        }
        if (matchCount == 0) {
            System.out.println(client.getClientName() + 
                "is compatible with the following client(s)");
        }
        System.out.println("\t" + otherClient.getClientName());
        matchCount++;
    }
    final boolean anyMatch = (matchCount > 0);
    if (!anyMatch) {
        System.out.println(client.getClientName() + 
            "is not compatible with any client.");
    }
    System.out.println();
}