What is the cleanest way of writing a function with conditional statement with many returning options?

Question

Maybe that's simple, but I'm a little confused.

I have such a Ruby code:

def my_function(found_objects)
  # ...
  if found_objects.second
    return CoreObjectFactory.get_object(found_objects.second, found_objects.first)
  else
    return CoreObjectFactory.get_object(found_objects.first)
  end
  # ...
end

I left return keywords for clarity, but there aren't the biggest problem.

~~According to Uncle Bob~~ (edit: I'm sorry, that was not his opinion, I messed something) every function should have only one ending, so I'm trying to refactor my code. The simplest solution is this:

def my_function(found_objects)
  # ...
  if found_objects.second
    result = CoreObjectFactory.get_object(found_objects.second, found_objects.first)
  else
    result = CoreObjectFactory.get_object(found_objects.first)
  end
  # ...
  result
end

But here Rubocop tells me:

Use the return of the conditional for variable assignment and comparison. (convention:Style/ConditionalAssignment)

It seems to suggest something like this:

def my_function(found_objects)
  # ...
  result = if found_objects.second
             CoreObjectFactory.get_object(found_objects.second, found_objects.first)
           else
             CoreObjectFactory.get_object(found_objects.first)
           end
  # ...
  result
end

Although in my opinion it's really creepy. Number of spaces intending this code is odd, what makes it looking bad and harder to work with Tab (nested intendations inteds only one space by default).

But I could write the same this way:

def my_function(found_objects)
  # ...
  result = if found_objects.second
    CoreObjectFactory.get_object(found_objects.second, found_objects.first)
  else
    CoreObjectFactory.get_object(found_objects.first)
  end
  # ...
  result
end

It looks better for me, but unfortunately not for Rubocop, which has three conventions against me. Of course I can ignore it... but it still seems not perfect.

Layout/IndentationWidth:
  Enabled: false
Layout/ElseAlignment:
  Enabled: false
Layout/EndAlignment:
  Enabled: false

I could also use a ternary operator, but Rubocop doesn't like long lines and multiline ternary opeators.

if found_objects
  # ...
  result = found_objects.second ?
    CoreObjectFactory.get_object(found_objects.second, found_objects.first) :
    CoreObjectFactory.get_object(found_objects.first)
  # ...
  result
end

So, is there some way to avoid all problems above?

Uncle Bob might well have repeated the old Dijkstra thing. My answer wasn't meant to tell you that he didn't say it, but rather that you shouldn't unquestioningly listen to him. — Alex Reinking, Oct 01 '19 at 00:08
Your first variant is fine, but so is your secend, and if a braindead style checker would tell me the second version is not, I would disable the particular style check rule. — Doc Brown, Oct 01 '19 at 03:50
Possible duplicate of [Style for control flow with validation checks](https://softwareengineering.stackexchange.com/questions/148849/style-for-control-flow-with-validation-checks) — gnat, Oct 01 '19 at 05:23
@AlexReinking - yes, I know you didn't mean this. I just checked it in the book and indeed, Uncle Bob was writing about this old rule, but didn't recommend it, claiming that sometimes it's better to have multiple return statements and that if we manage to keep our functions short, no problems should appear. — Karol Selak, Oct 01 '19 at 09:25
The way to avoid all problems above is to realize that you, as a developer working on this project, (or you and your team, if this is a team effort) have the power to make a judgement call and make up your own mind(s), and if that conflicts with someone else's opinion or Rubocop, so be it. — Filip Milovanović, Oct 01 '19 at 10:51

score 6 · Accepted Answer · answered Sep 30 '19 at 23:52

6

According to Uncle Bob every function should have only one ending

This is a weird corruption of an old rule from Dijkstra that says that functions should only have a single entry and a single exit. What this really refers to is in old Fortran and assembly language programming where you could enter the function at multiple places and when you leave the function, return to different places (ie. not the original call site). Note that this is not the same as returning to the same place from multiple different exit points within the function.

You should feel fine writing multiple returns if it's clearest to you and your team. Multiline ternary operators can be an eyesore and there doesn't appear to be a way to move the ternary inside the function call.

answered Sep 30 '19 at 23:52

Alex Reinking

1,607
11
16

1

Actually this rule also applies to modern languages. The reason returning to different places is an issue is because those places would all need to know what resources you've allocated that now need to be deallocated. Yuck. Do that in one place. It's why we have finally blocks. Stick as many return keywords as you like in a `try` with a `finally` block and your method still only has one exit point. The rule is a good rule. But only if you understand what it's about. Early returns are fine so long as you have some way to deal with this issue. – candied_orange Oct 01 '19 at 01:09
1

You can have multiple try-finally sequences in a function if you want to release some resources before acquiring a different set. That still achieves the real goal of cleaning up resources, but allows the function to have multiple exit points. – Alex Reinking Oct 01 '19 at 01:14
2

Yes, you can also get away with this by simply not allocating any resources. The point is to design code that can be maintained by those who come later who have to allocate some resources and would like to deallocate without having to completely rewrite the method. Having one place to do deallocation is nice. Using a design that makes it easy to construct that one place is nice. Spare us maintenance programmers a little thought please? – candied_orange Oct 01 '19 at 01:21
1

@candied_orange: using try-finally does not require a "complete rewrite", that is an exaggeration. And a function which does not allocate any resources is turned into one which does, it is usually necessary to change some bits and pieces. – Doc Brown Oct 01 '19 at 03:54
@DocBrown read what I actually said again. I'm asking you to consider a design that allows us to add a try finally, without a complete rewrite of the function. That's why I said use of multiple return statements can be ok. Just don't assume every possible design is ok so this rule can be ignored today. Some designs cause a need for a complete rewrite of the function on this exact issue because they don't allow creation of a single place to go to clean up. If you write long functions that make adding try finally difficult, you're ignoring maintenance needs. – candied_orange Oct 01 '19 at 09:51
It really doesn't apply to modern languages when the function in question doesn't acquire any resources. If you have some special cases, there is no reason to prefer wiring a value through the function, complicating the dataflow analysis for the compiler, to simply returning as soon as a value is computed. Or maybe you're doing a linear scan and want to return early. Why go through the set-a-value-and-break rigamarole when you can just return? More efficient and clearer. – Alex Reinking Oct 01 '19 at 18:30
The problem is fundamentally with resource management, not function control flow. It very often makes perfect sense to return early and contorting your code to fit this bogus rule, confounding readers and compilers alike in the process, helps no one. Think about C++'s RAII pattern. C++ has other problems, but "single exit" in this interpretation makes _no_ sense when clean up is so automatic. – Alex Reinking Oct 01 '19 at 18:32
@AlexReinking the end of the finally block IS the "single exit" even with early returns. I'm saying the rule shouldn't be ignored but it should be better understood. – candied_orange Oct 01 '19 at 23:56

score 5 · Answer 2 · answered Oct 01 '19 at 00:24

First code is meant to be read. If the code is hard for you to read, then even if millions of other people are quite happy reading the code, something is still wrong. Whatever you do choose to go with, it should be ledgeable to you first.

Second, RuboCop and all of the other style systems are formulaic. A lot of good code uses those same layouts and conventions that have been distilled into those formulas. But so does a lot of bad code. Just following those formulas turnkey will not necessarily make good code, just help you avoid some well known sources of bad code.

Third, Good code is prose. It is words and symbols that explain themselves as much by their content as by their layout. You yourself can see that in your own analysis. You intuitively know that the first and second examples communicate with you better than the if expression, or the ternary operator.

To avoid the problem above, stop adhering to styling advice blindly. Particularly when it reduces the simplicity or clarity of the code. This also means that RuboCop must be demoted to an advisory tool, it should not ever be used as a quality gate (in so far as its styling capacity). It is simply an aid to highlight code that could use some communication polish.

What is the cleanest way of writing a function with conditional statement with many returning options?

2 Answers2