18
\$\begingroup\$

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;
}
\$\endgroup\$
  • 31
    \$\begingroup\$ return "Engineers are never happy..."; \$\endgroup\$ – Jon Story Oct 14 '15 at 22:59
  • 3
    \$\begingroup\$ @JonStory that should automatically evaluate to true within Java, except the engineers were unhappy with this and refused to put it in. \$\endgroup\$ – DoubleDouble Oct 15 '15 at 17:50
  • \$\begingroup\$ Do not close. Code is real enough to get good reviews, as has happened. \$\endgroup\$ – Caridorc Oct 19 '15 at 21:46
35
\$\begingroup\$

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.

\$\endgroup\$
  • 9
    \$\begingroup\$ How about return engineers.stream().mapToInt(e -> e.isHappy() ? 1 : -1).sum() > 0; \$\endgroup\$ – rolfl Oct 15 '15 at 0:29
  • \$\begingroup\$ This is the most readable and natural coding expression of this idea. Does it without calling engineers.size(), also; very nice. \$\endgroup\$ – Wildcard Oct 15 '15 at 3:27
  • 3
    \$\begingroup\$ engineers.size() is generally (always?) implemented in O(1) time, so maybe you could return early if netHappiness is too large (resp. too low) for other engineers' mood to change the final outcome. I know, this is too clever ;-) \$\endgroup\$ – coredump Oct 15 '15 at 13:19
  • \$\begingroup\$ @coredump Unfortunately, not always. In Java's libraries it is, but I could make my own class that implements List, and not do so. It is always recommended that List#size be implemented in O(1) time, though. \$\endgroup\$ – Justin Oct 15 '15 at 14:42
  • 2
    \$\begingroup\$ @Justin You could also make your own class that implements List whose iterator method calls 911 and reports a hostage situation. Therefore, solutions that involve calling iterator must be bad. \$\endgroup\$ – user253751 Oct 16 '15 at 6:17
19
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Impressive, really cool. \$\endgroup\$ – Iaroslav Karandashev Oct 14 '15 at 22:34
  • 6
    \$\begingroup\$ That's less readable... (Although could be formatted to be more readable) \$\endgroup\$ – Jon Story Oct 14 '15 at 23:02
  • \$\begingroup\$ Yes your are right adding some break lines will improve it. But I still think is more readable without taking how is formatted into account. \$\endgroup\$ – MAG Oct 14 '15 at 23:05
  • \$\begingroup\$ Nice, but anyone who can understand that answer, will also understand @mdfst13's answer, so I would go with mdfst13's \$\endgroup\$ – matt freake Oct 15 '15 at 14:01
15
\$\begingroup\$

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();
\$\endgroup\$
  • 3
    \$\begingroup\$ I'd go with happyCount over numHappy. \$\endgroup\$ – Celos Oct 15 '15 at 6:58
  • \$\begingroup\$ @Celos That's definitely a better name. \$\endgroup\$ – Justin Oct 15 '15 at 6:59
13
\$\begingroup\$

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 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.

\$\endgroup\$
  • 1
    \$\begingroup\$ you are counting not happy engineers if (!engineer.isHappy()) { happyEngineers++; } \$\endgroup\$ – MAG Oct 14 '15 at 22:56
  • 1
    \$\begingroup\$ That section is an English Language problem, as well as a code problem. Avoid double negatives! \$\endgroup\$ – Jon Story Oct 14 '15 at 23:01
  • \$\begingroup\$ @MAG good catch, will fix it. \$\endgroup\$ – TheCoffeeCup Oct 14 '15 at 23:01
  • 3
    \$\begingroup\$ Java conventions require indents to be 4-spaced, not 2-spaced, like almost any other language - Citation needed! I've worked with plenty of projects, in C, Java, whatever, that wanted 2-space indents. My company currently uses 2-space indents. The size of indent is not important, as long as it's self-consistent, and meshes with the style of whatever other code is nearby. \$\endgroup\$ – amalloy Oct 15 '15 at 1:05
  • 1
    \$\begingroup\$ @MannyMeng I'd go with the name isMajorityHappy rather than majorityEngineersHappy as it is already clear from the method parameters that we're talking about engineers. And since happiness isn't very engineer-specific, I'd probably end up with an interface Mood with method isHappy, which is then implemented by the class Engineer. \$\endgroup\$ – Erwin Bolwidt Oct 15 '15 at 2:09
2
\$\begingroup\$

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 !)

\$\endgroup\$
  • 2
    \$\begingroup\$ This doesn't address any of the question. Pre or post-decrement won't make a difference, and it doesn't use any arrays. Furthermore, where it doesn't make a difference, post-increment is used more commonly, so using pre-increment for no reason is less clear. \$\endgroup\$ – Joe Oct 16 '15 at 11:02
  • \$\begingroup\$ Using post-increment should be considered when you need to use this feature, otherwise it is useless, and a source of error (when looping on an array), and the fact that more people are used to it is not a good point. My answer was a tip to make OP think of the best practices, it could have been a comment to its post but I wanted to add examples. \$\endgroup\$ – Tom Oct 17 '15 at 10:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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