• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Bear Bibeault
  • Devaka Cooray
  • Liutauras Vilda
  • Jeanne Boyarsky
Sheriffs:
  • Knute Snortum
  • Junilu Lacar
  • paul wheaton
Saloon Keepers:
  • Ganesh Patekar
  • Frits Walraven
  • Tim Moores
  • Ron McLeod
  • Carey Brown
Bartenders:
  • Stephan van Hulst
  • salvin francis
  • Tim Holloway

Make nested if statements more elegant  RSS feed

 
Ranch Hand
Posts: 174
2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm a Jr. Java Dev and I wrote the following method. However, my lead asked me to "tidy-up" those if statements. It was hard enough for me to get the code right for this particular task... and I'm at a loss as to how I could make this cleaner looking... is there something simple that I am missing?

 
Sheriff
Posts: 23874
50
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It appears that the intention of the code inside the for-loop is to add at most one office's room to a list. And there are various conditions on when that adding should take place, which leads to the complex set of if-statements which you have there.

I notice that line 13 in the first if-group and line 20 in the second if-group are identical. So you're not going to add any records unless office.getFacility().equals(dropdownChangeFacility) is true. So you could pull that out and wrap it around the two if-groups, reducing their if-conditions accordingly.

But there's something which bothers me about that idea: if the first if-group you're concerned about whether dropdownChangeFacility is null and in the second if-group you aren't concerned about it. Is it actually true that office.getFacility() might return null, but sometimes that excludes the office from being in the list and other times it doesn't? I'm suggesting that maybe the if-test made up of lines 11 and 16 might be unnecessary, or alternatively that it could be pulled out to the higher level as well.

It would also help a lot to include some comments which explain why those particular tests are being made. My interpretation, based on staring at the code for a quite a while, is something like this:

1. If the office's facility matches the dropdown change facility, and there's no dropdown change floor, then the office goes in the list.

2. If the office's facility matches the dropdown change facility, and the office's floor matches the dropdown change floor, and the office has workspaces, then the office goes in the list.



When I write it that way it becomes clearer that matching the dropdown change facility can perhaps be done up front, which simplifies the two tests there.

Does that help?
 
Bartender
Posts: 19989
95
Android Eclipse IDE Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
"Tidy Up" can be taken 2 ways.

As far as formatting goes, I'd get rid of most of the double-spacing, since it spreads the logic out and makes it harder to see all of it in a single glance. Not to mention makes it less likely to all fit on the screen at the same time.

As far as logic goes, Paul has given some guidelines.

I've always sneered at the assertion of the ignorant that you have to know mathematics inside and out to do programming (unless you're programming for math), but there is one branch of mathematics that all good programmers would be well-advised to be fluent in, and that's the Calculus of Propositions.

Propositional Calculus (PC, for short) is nothing like numerical calculus. Instead it deals with the rules that allow rewriting logical expressions into other, equivalent (and hopefully simpler) logical expressions just like you would simplify algebraic expressions. These rules are named (de Morgan's Theorem, modus ponens, modus tollens) and have been mathematically proven.

Not only can you use them to simplify statements and expressions, but you can also invert logic to make it more understandable. Several years ago, I read that a college professor dedicated an entire term to "not", because negation throws up a mental "speed bump" when trying to comprehend things.

IBM was a bad offender. The original OS/360 mainframe Job Control Language managed conditional execution of job steps using expressions that basically were coded in the form "if x is true then do not execute y".
 
Java Cowboy
Posts: 16084
88
Android IntelliJ IDE Java Scala Spring
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Note that:

is the same as:

 
Tim Holloway
Bartender
Posts: 19989
95
Android Eclipse IDE Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's a useful construct that will cut down on verbosity and MAYBE even add efficiency:



The downside being that you have to know and be willing to hard-code a suitable datatype for the declared variable officeRoom.

I generally like to make my collection objects be always empty instead of null, also variables and lists you get from GUIs are often never null. But admittedly it never hurts to be paranoid. Not only can unexpected data be encountered, but overly-optimistic assumptions can be exploited by attackers.
 
Bartender
Posts: 9494
184
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm guessing you can also make the code easier to read by negating some conditions and using early returns/continues.
 
Sheriff
Posts: 12748
210
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
This has arrow code painted all over it and your lead was right to ask you to clean it up but IMO, fell short of his responsibility to help you learn how to do it properly. I would do a bunch of extract methods. The code you wrote is full of formulas that don't clearly express what they mean.

Look at this code:

You should recognize that it's functionally equivalent to this:

Why? The if statement on line 11 is unnecessary because by contract equals(null) always returns false. So right there, you can eliminate one level of nesting.
 
Junilu Lacar
Sheriff
Posts: 12748
210
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Another thing with the way you wrote that method is that it can apparently return null (line 33).  When a method is declared as returning a List of anything, it is very poor form to make the method return null. This forces your client code to check for null to avoid causing a NullPointerException. If you declare a method that returns something like a List<String>, then the least you should return is an empty List<String>.
 
Junilu Lacar
Sheriff
Posts: 12748
210
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I don't know what exception you're expecting but since you declared your catch block to catch Exception, then I'm going to assume the worst and guess that the try-catch block is speculative and born out of fear of the unknown. That is not good. The only part of the code that looks like it might throw an exception would be new OfficeManager().getOffices(). If you don't know exactly what exception that call might throw, then you need to find out. Don't cop out and just have a generic try-catch (Exception e) block wrapped around that entire section of code. You can do better than that.

Looking at the code some more, I would probably try refactoring that code to something like this (assuming Java 8):
 
Junilu Lacar
Sheriff
Posts: 12748
210
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you're not using Java 8 or greater, then you can still keep the code relatively clean like this:
 
Junilu Lacar
Sheriff
Posts: 12748
210
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would also question whether the list of Office objects returned by OfficeManager.getOffices() can actually return different objects that have the same value for getRoom(). I don't see why it would. If, however, it is somehow possible, then you can use a Set to collect the values of getRoom() first, then return a List created from that Set.

This points to a subtle code smell in your code: the mixing of concerns.

You have two major concerns in this method: 1) Selecting Office objects that match the filter criteria and 2) excluding duplicate values returned by getRoom(). Your conditional logic becomes more complicated because it incorporates the two concerns. A better separation of the two concerns simplifies your conditional selection logic. By using a Set to collect values returned by getRoom(), you move the responsibility of eliminating duplicates to an object that already inherently does that.
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!