
Why Swift Guard Should Be Avoided - ingve
https://medium.com/swift-programming/why-swift-guard-should-be-avoided-484cfc2603c5
======
moonshinefe
Has anyone seen production code with <=10 lines of code per function before?
I've honestly never seen it, and I think at that point the code becomes harder
to read because you're tracing through function definitions to decipher what a
single function should do.

I usually notice functions get hard to read when they hit 80 lines or so long
(generalization, of course), and try to keep mine under say 50 unless it's one
of a few core functions in an application.

Interested in hearing others' experiences.

~~~
krylon
> Has anyone seen production code with <=10 lines of code per function before?

I have seen code where _a_ function was shorter than 10 lines. ;-) In that
same code base, I have seen _a_ function that was ~750 lines. And for that
particular function, it actually made sense. It involved a lot of local state,
and breaking it up would have involved passing lots of parameters to the new,
shorter functions. (The average function in that code base was around 50-60
lines, I guess.)

Using a fixed number, like "10 lines" seems arbitrary to me. Sometimes it
makes sense, sometimes it does not. It also ignores other factors, such as
comments. Or readability. Sometimes, I 100+-line function can be marvelously
clear, other times, a 5-liner can be so dense it makes my eyes bleed.

I have grown wary of "Always do this"/"Never do that"-advice.

"Try to keep your functions short. If they grow long, look if it makes sense
to break them up into multiple shorter functions" is sound advice. But to do
so without questioning or thinking is problematic.

~~~
SideburnsOfDoom
> I have seen a function that was ~750 lines. And for that particular
> function, it actually made sense. It involved a lot of local state, and
> breaking it up would have involved passing lots of parameters to the new,
> shorter functions

If I was refactoring it, and 750 lines is a massive red flag that it is in
need of refactoring; then the local state indicates that it becomes not a
function or a bunch of functions, but a class with state and private methods.
Is that hard to spot?

~~~
moonshinefe
I actually missed that one part of his response when I agreed in reply. 750
lines is definitely a red flag for refactoring.

~~~
krylon
Well, I agree that 750-ish lines is a bit extreme, but that function was a
Win32-message handler. It basically consisted of a _huge_ switch statement
that figured out what the message was, did a little parameter mangling and
then called a function that did the actual work.

So while that function was very, very long, it was relatively simple to
understand and did not contain a lot of logic. Also, the fact that it was a
huge switch would have made breaking it up slightly non-trivial.

~~~
SideburnsOfDoom
Yes, the "big switch" is the canonical example of something that's hard(er) to
break down and easy(er) to read as 1 function.

Though I would still try things like :

\- making each case in the switch as small as possible, preferably a 1-liner.

\- making a separate object that does the "parameter mangling".

\- grouping all the bits of state into an object to hold the "message loop
state".

\- Or most radically but best: these "map", "lookup", "strategy" style
patterns that reduce it to a table of (message id, handler fn) pairs
[http://stackoverflow.com/a/126453](http://stackoverflow.com/a/126453)

~~~
krylon
It was years ago that I worked at that company. Unfortunately, things like
refactoring weren't very big there, and the other developers - who were, I
should emphasize, really nice guys! - were not exactly open to change.

When I was hired there, that application had been in development for ~15
years, and a lot of what I did was maintenance. So making coming in as the new
guy, then starting to change things, I had to tread rather carefully.

FWIW, I got each of the three developers to individually admit to me that if
we had the time and the money, we would basically throw the old code out and
rewrite it from scratch, using the lessons they had learnt since. But time and
money being what they were, that was not likely to happen. (I was really proud
I got them to admit that, though - one of the other developers was _very_
defensive of his work, and getting him to admit that was quite an achievement;
I actually gained some valuable people skills in doing so.)

I was glad, though, that the code was relatively readable and organized in a
sane manner. There was some cruft that had accumulated over the years, but
overall it was surprisingly sane. That was one of the first times I had to
read somebody else's code, and it was good code for that purpose - sparsely
commented, yet very clear.

Also, keep in mind that this was C. Different languages offer different ways
of decomposing / structuring code.

------
voltagex_
>What previously was a thirty-line function with several indentation levels
and intermediate variables that had to be kept in the head is now ten
functions, each having a self-documenting specific name.

My gut feeling is this wouldn't pass the unofficial code reviews at my work -
too many functions making the code look messy, instead of cleaner.

Does anyone have any counterpoints?

~~~
static_noise
To me the general idea of splitting a complex problem into a series of less
complex problems is good. However the example used seems trivial and the
solution is worse than the problem:

> 1\. It is pretty long, sixteen lines, multiple parts separated by empty
> lines.

I wouldn't call 16 lines "long".

It contains empty lines for easier reading. Well done, not actually a problem.

But it lacks any comments on why the assertions have to be made.

> 2\. It does several things. It gets an item by name, validates parameters,
> and it implements the logic of vending an item.

These are the things that naturally belong together. Separating validation
from logic is not a very good idea. The logic requires the specific assertions
in the validation part. They belong together.

> 3\. It has several abstraction levels. The high-level vending process is
> hidden among the finer-level details like Boolean checks, usage of specific
> constants, math operations, etc.

In the single function implementation it's actually not hidden because it's
all in one function. Use some comments to structure your function.

\---

In my opinion separating this function into multiple functions would only make
sense if these parts are used multiple times AND if they are commented well on
why they need to act this way.

~~~
SideburnsOfDoom
> However the example used seems trivial and the solution is worse than the
> problem:

That's more or less the problem when discussing how to work with complex, real
world software and giving examples of how to do it: the examples end up being
simple enough that the techniques aren't worthwhile in that case. Don't
mistake that for the techniques not being worthwhile ever.

I have seen the same when code is presented showing examples of how to use an
IoC Container. The response "but this overcomplicates the code" is true, but
misses that it doesn't apply to the (far larger) codebase that IoC containers
are aimed at.

------
jernfrost
This is just nuts. If you disregard the curly braces each of these guards are
just two lines of code. When you place them into a function definition they
each become 3 lines of code. Just so you can replace them with single lines in
the original code.

That isn't software engineering. That is religious fanaticism.

E.g.

    
    
            guard item.count > 0 else {
                throw VendingMachineError.OutOfStock
            }
    

Is a very low complexity statement. One comparison is made and an enum value
is thrown. You are gaining no clarity by placing this inside a separate
function.

~~~
golergka
This is an oversimplified example. When I imagine how both approaches would
look like in actual production code with much more complex requirements, it
makes much more sense.

------
lassejansen
Related discussion by John Carmack:

[http://number-
none.com/blow/john_carmack_on_inlined_code.htm...](http://number-
none.com/blow/john_carmack_on_inlined_code.html)

[https://news.ycombinator.com/item?id=8374345](https://news.ycombinator.com/item?id=8374345)

Cached:

[http://webcache.googleusercontent.com/search?q=cache:LQgMUMT...](http://webcache.googleusercontent.com/search?q=cache:LQgMUMTnhn0J:number-
none.com/blow/john_carmack_on_inlined_code.html)

------
jakobegger
A great example of how you can write spaghetti code without using goto: Just
split a simple function into a dozen functions with generic names like
"validate", and you've made sure no-one ever will comprehend your code.

------
panic
Lots of short functions are usually worse than a single long function. When
you go to figure out what the code is doing, instead of reading through each
statement of a single function and understanding it as you go, you keep being
interrupted by new functions you have to read and understand. What does
"reduceDepositedCoinsBy" do? Yes, I'm sure it subtracts its argument from the
deposit amount, but what _else_ does it do? Could it be causing the bug I'm
trying to fix? No, it just does the obvious thing, OK, now what does
"removeFromInventory" do? And so on…

~~~
gnufied
So, leaving aside the fanaticism around number of lines a functions should
have which I disagree with, there are some big advantages of splitting a
functions in logical units.

1\. If you have ever written unit test for 300 line function you know it gets
dicey. Such a function will likely require too much setup and will require
complex mocks/patches/doubles to be unit tested.

2\. As I mentioned before, I don't believe in number of lines thingy but
logical unit of separation. If a function, opens a file, reads & parses config
data from it, reads URL from the config data , parses http response and writes
updated bits to config data - these distinct things can be logically separated
and will be easier to unit test perhaps (again production scenario might be
more complicated). Also, with modern IDEs and editors jumping back&forth
between functions should be a breeze and your IDE/editor should provide an
outline of file which can be scanned quickly.

3\. If you have written a long function that does too many logical things(not
again by number of lines) - next person who makes changes to it, isn't going
to break it up - even if breaking up makes sense. For example, lets say in
above example original code was written with expectation of certain format of
config data but now somehow the function has to support more than one config
data. Now the code is so tightly coupled that - it is harder to break out
parser of this new format to new function and hence the function will keep
growing (and did I mention the unit test setup has to support new config
format as well somehow).

3\. If however, lets say method computes moving average of a timeseries, it
probably makes sense to have it all in one function even if it becomes
slightly longer.

TL;DR - Cargo culting must be avoided at all costs, however we don't need to
throw the baby with the water. Each problem calls for our own judgement.
Separate the functions by logical units, rather than number of lines, use
proper editor/IDE.

~~~
SideburnsOfDoom
> Each problem calls for our own judgement.

Indeed it does, but if you have never even tried to go full "Uncle Bob" and
write really small SRP functions and classes, then your judgement won't be
well-informed. You'll be surprised how well it can work if you've never tried
it. It works for readability, maintainability and testability.

Statements like the grandparent's "Lots of short functions are usually worse
than a single long function" IMHO mostly show a lack of relevant experience.

~~~
gnufied
Let me come out and say - I think OP is wrong. When people write 500 line long
functions the only way they are going to test it is, run with real input and
see if it breaks.

Such testing is not only insufficient, but any regression that is added as a
result of a change will go unnoticed too, until a real user hits that bug. You
will notice how most people who are supporting long functions - don't even
talk about how the heck they are going to test it.

------
rimantas
That says nothing about the guard itself, only about code organization. Ifs in
some of his refactored functions can be as well guards. I think this is much
better: [http://radex.io/swift/guard/](http://radex.io/swift/guard/)

~~~
cellularmitosis
Exactly. Here's a version of his refactored code which re-replaces the if
statements with guard statements:
[https://gist.github.com/cellularmitosis/9a175facd5fe260403f1](https://gist.github.com/cellularmitosis/9a175facd5fe260403f1)

------
alexjarvis
I agree that you can split validation parts into separate functions but this
is conflating two concerns, for example the rewriting of the guard statement
from

    
    
      guard item.count > 0 else {
        throw VendingMachineError.OutOfStock
      }
    

to

    
    
      if count == 0 {
        throw VendingMachineError.OutOfStock
      }
    

only proves that you can refactor the code to not use guard whereas the
article starts by talking about small functions, where you could in fact still
use guard in this case.

~~~
rimantas
Yes. And if all your code is composed of three-lines methods, then ok.
Otherwise guard is extremely usefull, because unlike 'if' it leaves your
constant/variable accessible outside the guard scope.

------
mhd
If an article starts with basically "Because Uncle Bob said so", _cum grano
salis_ doesn't even cover my mindset when reading the rest of it.

Function size has been a pretty peculiar fetish. Even when viewed on the
mythological 80x24 remote terminal, the initial function doesn't seem to be
that hard to understand, and spreading it out between some function with
lackluster naming doesn't really improve it all that much ("Computer science
has two hard problems: cache invalidation, naming things and off by one
errors").

Literate programming or the rather esoteric "refinements" at least had the
advantage of being able to use proper sentences to describe the functional
component and with the former you even had the option of being able to look at
the code in both forms.

Having said that: As I'm not a Swift programmer (ba-doom-tish), is this all
the language has that approaches Design By Contract?

~~~
SideburnsOfDoom
> If an article starts with basically "Because Uncle Bob said so", cum grano
> salis doesn't even cover my mindset

Uncle Bob is a recognised expert in the area. So no, his opinions on SRP
should not just be dismissed out of hand.

------
therockhead
Another take on guard [http://ericasadun.com/2016/01/01/another-take-on-
guard/](http://ericasadun.com/2016/01/01/another-take-on-guard/)

------
lassejansen
I don't think it's useful to extract every guard statement into a separate
function. Consider importing a json file where each field could potentially
contain invalid or missing data. Sure, you could create functions like
parseTitle, parseSubtitle, parseAuthorName etc. but these functions will
probably only be called from one place and I don't think they improve
readability.

On the other hand, I do think that larger functions should be broken up into
smaller parts where it makes sense, e.g. a separate function for parsing each
object in the json example.

------
SuddsMcDuff
All too often, in every stack, the language is blamed for design deficiencies
which lie with the programmer.

------
andrewchambers
It seems foolish to take function length as a standard of good code. The
example refactor looks horrible to me.

How about the function can be read top to bottom, doesn't mess with global
state (unless it explicitly is documented as doing so), and has clear inputs
and outputs. Who cares if it is 5,10 or 100 lines if it does that.

------
Dylan16807
So the example of a function that's too long, because functions should be 10
or fewer lines, is a function that has exactly 10 lines with code on them.

------
gelasio
Guard makes things less readable because it "hides" variable declarations. If
you break it out onto it's own line I'm sure it would make things look
downright ugly.

"Don't put multiple statements on a single line unless you have something to
hide" \-
[https://www.kernel.org/doc/Documentation/CodingStyle](https://www.kernel.org/doc/Documentation/CodingStyle)

(If you disagree or if you think I said something wrong and my comment
deserves to be hidden, I'd love to know why!)

------
realo
This article seems to confuse clarity brought by folding code with versatility
brought function reuse.

