Check if more engineers are happy than not happy

Question

I wish to use this sample to make my code more readable. Could you share with me your ideas on method naming, variable naming, if-statements etc.

It would be great if you have any URLs.

private boolean areEngineersHappy (List<Engineer> engineers) {
  int notHappyEngineers = 0;
  int happyEngineers = 0;
  for (Engineer engineer : engineers) {
    boolean notHappy = !engineer.isHappy();
    if (!notHappy) {
      happyEngineers++;
    }
    if (notHappy) {
      notHappyEngineers++;
    }
  }
  return happyEngineers > notHappyEngineers;
}

Answer 1

Consider

private boolean isMajorityHappy(List<Engineer> engineers) {
    int netHappiness = 0;

    for (Engineer engineer : engineers) {
        if (engineer.isHappy()) {
            netHappiness++;
        } else {
            netHappiness--;
        }
    }

    return netHappiness > 0;
}

Name the method based on what it does. What it does is check if the majority of the members of the list are happy. The fact that the list is of engineers is mostly beside the point.

I would expect areEngineersHappy to return false if any engineer is unhappy.

Note that we don't need two variables to tell if the majority is happy. We can use a single variable and subtract for unhappy members of the list. If netHappiness is positive, then the majority of the members of the list are happy.

I would try to avoid constructs like if (! notHappy) {. It would be much simpler to change the logic around so that you can say if (happy) { instead.

You don't need to say if (happy) and if (! happy). This is exactly the situation for which an else is designed.

Answer 2

Adding to Manny Meng answer if your main concern is readability and Java 8 is available you may want to use streams.

Something like this:

return engineers.stream().filter(Engineer::isHappy).count() > engineers.size() / 2;

EDIT (Due to some valid point in the comments regarding formatting. I myself don't like large one liners either):

int numHappy = engineers.stream().filter(Engineer::isHappy).count(); 
return numHappy > engineers.size() / 2;

Answer 3

I'm going to try to avoid repeating what people have already said.

• If you look carefully at your code, you'll notice that nowhere do you access state. As it is, this function doesn't really belong to an object; it would be better as a static function:

  private static boolean mostlyHappy(List<Engineer> engineers) {
  

• What if someone had a class SoftwareEngineer that extends from Engineer? In that case, I cannot pass my List<SoftwareEngineer> to your function. So let's make that change as well:

  private static boolean mostlyHappy(List<? extends Engineer> engineers) {
  

• Variable names (as you specifically requested this):

  • int notHappyEngineers. Since "not happy" is equivalently "unhappy", I personally think it would be better to call this int unhappyEngineers. However, if I read unhappyEngineers, that doesn't tell me much about what it stores. I'd instead name this int numUnhappy. Similarly with int happyEngineers.

    int numUnhappy = 0;
    int numHappy = 0;
    

• Java 8. While your code is already reasonably readable, I think that using Java 8's streams will further improve readability:

  private static boolean mostlyHappy(List<? extends Engineer> engineers) {
      int numHappy = engineers.stream()
                              .filter(Engineer::isHappy)
                              .count();
      return numHappy > engineers.size() / 2;
  }
  

When using Java 8's streams, I believe it is important to put each operation on its own line. This makes it quick and easy to see what the code does, since I only need to read vertically, rather than try to parse horizontally.

Taking the above code as an example, here's my thought process upon reading it:

1. filter
2. count. Oh so we are counting the number of elements that match filter's predicate.
3. Engineer::isHappy. We are counting the number of happy engineers. Great, that makes sense.

The indentation for the stream operations themselves is debatable. I'd probably end up with it looking something like this in my own code:

int numHappy = engineers.stream()
                   .filter(Engineer::isHappy)
                   .count();

Answer 4

Some points:

1. Java conventions require indents to be 4-spaced, not 2-spaced, like almost any other language. (Source)

2. The space between the method name and the parentheses can be removed:

   areEngineersHappy (List<Engineer> engineers)
   

3. areEngineersHappy is not really a good name for a method. I would suggest perhaps majorityEngineersHappy(), but even mine I am not happy about.

4. Some code can also be simplified:

   boolean notHappy = !engineer.isHappy();
   if (!notHappy) {
       happyEngineers++;
   }
   if (notHappy) {
       notHappyEngineers++;
   }
   

   can easily be:

   if (engineer.isHappy()) {
       happyEngineers++;
   } else {
       notHappyEngineers++;
   }
   

   or to even remove the notHappyEngineers variable:

   private boolean areEngineersHappy (List<Engineer> engineers) {
       int happyEngineers = 0;
       for (Engineer engineer : engineers) {
           if (engineer.isHappy()) {
               happyEngineers++;
           }
       }
       return engineers.size() / 2 < happyEngineers;
   }
   

If you are looking for java standard conventions, this would be a good place to start. It is quite outdated, so if you are looking for more up-to-date conventions, look here.

Answer 5

All the answers are fine, but I'd like to complete these with a simple point : when you use var++, your variable is incremented after the instruction ends. So, if you have :

int var = 12;
int result = var++; // Here, result is 12, whereas var is 13

You should consider using :

int var = 12;
int result = ++var; // Here, both result and var are 13

It can make your code clearer, especially when accessing an array.

// array = [12, 32, 42]
int t = 0;
System.out.println(array[t++]); // Prints 12, and afterwards, t is 1
System.out.println(array[++t]); // Prints 42, t is immediately 2

(Plus, it makes you save some operations on your CPU, but it's far-far-fetched !)