Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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("");
    }
}
share|improve this question
add comment

1 Answer 1

up vote 2 down vote accepted
  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();
}
share|improve this answer
    
Thanks for all your advice. I did originally use methods. Was this the correct way to do it: If client[a].gender != client[b].gender then call method compareAge, passing as parameters the actual array and the two client elements. –  Andrew Martin Nov 17 '12 at 20:55
    
Also, where would you declare the local variables? I declared them at the very start because they are used after every iteration of the first second for loop (meaning if I declared them after first for loop they would be re-declared everytime, thus generating an error). –  Andrew Martin Nov 17 '12 at 20:56
    
And forgive me, but I have to ask a third question! In your first comment, you mention creating different methods. Why have you passed the objects as 'final'? Why not just pass them as Client clientone? –  Andrew Martin Nov 17 '12 at 21:01
    
With regards to your three optimization ideas, from my understanding of our lecturer's instructions, we have to use just one array (an array of objects - our first time doing this). Also, the array is currently arranged by gender, so I don't know if it is worth sorting it by age, as although it will speed up age comparison, it would slow down gender comparison? –  Andrew Martin Nov 17 '12 at 21:08
    
@AndrewMartin: See the edit, please. Feel free to ask if you have further questions or miss one. –  palacsint Nov 17 '12 at 22:49
show 2 more comments

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.