Why Swift Guard Should Be Avoided

https://medium.com/swift-programming/why-swift-guard-should-be-avoided-484cfc2603c5

by ingve • 11 years ago

24 62 11 years ago

voltagex_• 11 years ago

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?

KarlFreeman• 11 years ago

I feel like somewhere between the two would pass. Keep the validation as distinct functions but inline validatedItemNamed, reduceDepositedCoinsBy.

static_noise2• 11 years ago

My problem with putting the validation in an own function is that this should only be done if the validation itself is generic like "check checksum" but not if it relates to the actual logic like "A must be smaller than B".

At first I wouldn't expect a function "validatedItemNamed" to check the price. While it checks the price it doesn't check the inventory number.

deanclatworthy• 11 years ago

I feel the same. When I'm new to a code base its a pain to keep jumping around functions to see what they actually do. I agree functions should be short but this is a step too far for me.

static_noise2• 11 years ago

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.

SideburnsOfDoom6• 11 years ago

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.

alexjarvis2• 11 years ago

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
  }

  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.

rimantas3• 11 years ago

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.

rimantas3• 11 years ago

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/

cellularmitosis• 11 years ago

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

panic• 11 years ago

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…

alexjarvis2• 11 years ago

Yes it's more cognitive overhead but it's a more functional than imperative style which usually has many advantages. If the functions are named well (naming being the hardest problem) then it should be easy to read without knowing exactly what the implementation of the function does.

Dylan168075• 11 years ago

I fail to see how reduceDepositedCoinsBy is in any way more functional.

w0utert• 11 years ago

I agree. Obviously 'god functions' and 'god classes' are bad, but splitting up tightly coupled, related logic into piles of <10 line functions seems ludicrous. Not only does it hurt readability (and debugging) like you mentioned, but it also promotes false sharing of functionality. By splitting things up into very small functions many things may start to appear reusable. The moment it turns out one or two instances of the code that calls all these small functions need to have subtly different behavior you'll be hunting down each and every piece of code that calls the function, to be sure that changing it won't break completely unrelated parts of the code.

gnufied3• 11 years ago

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.

Dylan168075• 11 years ago

Cargo culting must be avoided at all costs, however we don't need to throw the baby with the water.

But at the same time, just because you can transform someone's point into an ultra-generic form that's true, doesn't mean their original point has any validity. When someone gives you bathwater, don't spend too long looking for babies that aren't actually in it.

SideburnsOfDoom6• 11 years ago

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.

SuddsMcDuff3• 11 years ago

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

lassejansen2• 11 years ago

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.

moonshinefe5• 11 years ago

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.

Etheryte• 11 years ago

Likewise, agreed. Very weird to see this article score high on HN.

lmm• 11 years ago

Yes. Seen and written. Maybe it's easier with a small team, and it's certainly easier with a good language (Scala in my case) but it's absolutely possible.

jakobegger2• 11 years ago

Maybe people just don't handle errors? Most of my functions are less than 10 lines, but then you add the error handling code, and it becomes 30 lines.

Also, it strongly depends on the code you are writing. Things like reading/writing data often end up as pretty long functions, while glue code between model and view is usually just a couple of lines.

SuddsMcDuff3• 11 years ago

My general rule of thumb is that a function should be small enough to be entirely visible on screen without having to scroll. It's dependent on screen resolution & font size of course, but I find that having to scroll down adds some cognitive overhead to understanding the whole of a function.

Following this line of thought I've become a lot more precious about vertical whitespace & I've adopted BSD style curly braces.

moonshinefe5• 11 years ago

Fair enough. I always found horizontal scrolling more difficult than vertical though.

But, for instance, the Python PEP8 spec saying 80 characters max, I think is kind of ridiculous these days. Who is programming on their phone, tablet, or 90s computer that can't handle 120 characters these days?

Edit: I just mention 120 as PyCharm's PEP8 linter 'erroneously' checks that max rather than 80

jernfrost2• 11 years ago

My impression is that it is somewhat influence by the programming language. Code I've written have often needed so many variables to do its job that it is very hard to split out small functions. But when you can use closures and higher order function easily, I find it easier to keep the size of functions down.

But I prefer Linux take on function size:

The maximum length of a function is inversely proportional to the complexity and indentation level of that function. So, if you have a conceptually simple function that is just one long (but simple) case-statement, where you have to do lots of small things for a lot of different cases, it's OK to have a longer function.

krylon3• 11 years ago

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.

moonshinefe5• 11 years ago

Agreed.

SideburnsOfDoom6• 11 years ago

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?

rimantas3• 11 years ago

You might want to see this: https://www.youtube.com/watch?v=npOGOmkxuio Not really about production code, but still, some food for thought. Also this: https://www.youtube.com/watch?v=8bZh5LMaSmE

moonshinefe5• 11 years ago

She seems like a great speaker, will bookmark for later. TY.

SwellJoe• 11 years ago

Those are really excellent. I don't work in Ruby, but her explanations of OO design and refactoring are among the clearest, and most concise, I've ever seen. I'm gonna be watching her talks for the next couple hours, I suspect.

merb• 11 years ago

I've written 2 line functions already. It depends. If you write scala or any functional language and you do pattern match it's sometimes more readable if you have something like

    blabla match {
        case 1 => func1
        case 2 => func2
    }

instead of

    blabla match {
        case 1 => 
            println("1")
            1 + 2
        case 2 => biggerFunc
    }

jt2190• 11 years ago

The discussion is more fairly framed as: Do you use abstraction (hide details) in your code base? Number of lines of code is, perhaps, a symptom of code that doesn't use abstraction well, but isn't proof. We need to see the the code base, understand its design goals, and how it's actually used before we can pass judgement.

hawkice2• 11 years ago

I used to stick religiously to <=15, in C#. This was detrimental to understanding only twice, and a fairly small issue even then. Not a ridiculous standard for interns who don't have an excellent refactor-sense yet, so I didn't begrudge it back then.

Dylan168075• 11 years ago

But in sticking to that standard, what was the effort balance between improving understanding vs. busywork to meet the guideline?

Edit: Huh? Why a downvote?

For a rule made with the goal of improving understanding, it's great that the rule is not actively detrimental to understanding.

But we still need to know how often it actually succeeds vs. having marginal effect.

We also need to know what it costs in terms of labor.

Without those two bits of info we have absolutely no idea if it was a good rule.

eric_bullington• 11 years ago

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

It depends on the language. In Node.js code, it's not uncommon to not only see production code that follows this rule, but also puts no more than 1 or 2 functions in each file. Once you get used to it, and as long as you're quick with your editor/IDE's open shortcuts, I've found it a pretty nice way to organize code.

I personally find C code hard to follow sometimes, not because I don't understand C (I have more experience with C than I do JavaScript, if you count years), but because C tends to feature multiple functions per file, each with hundreds of lines of code per function. At least for me, that's hard to follow, and I hate having to move hundreds of lines up and down a file, even using Vim shortcuts.

SideburnsOfDoom6• 11 years ago

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

I've seen code where many functions were like this, and very few were over 30 lines. It was easy to read. And it was tested and generally working well. Naming becomes important, but that's part of the point of extracting functions for readability.

whiddershins• 11 years ago

In my personal experience, the small function thing can drive a maintainer bananas.

When trying to understand code I haven't written, I have had the experience that trying to trace logic and debug through a bunch of little functions is more cognitive overhead than just seeing everything that happens within one larger function.

I get the idea that small functions are supposed to be wonderful, and clear, if everything is named really well and everything works as the name implies.

But for me the downside is it ends up feeling like detective work. I have to jump around and remind myself exactly how each little function works just to figure out what exactly is happening.

My dream IDE would let me see "expand" every function call ... like with little dropdown arrows .. so I could see it as if it were inline, and could trace execution linearly in one solid block.

lassejansen2• 11 years ago

Related discussion by John Carmack:

http://number-none.com/blow/john_carmack_on_inlined_code.htm...

https://news.ycombinator.com/item?id=8374345

Cached:

http://webcache.googleusercontent.com/search?q=cache:LQgMUMT...

jakobegger2• 11 years ago

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.

jernfrost2• 11 years ago

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.

therockhead2• 11 years ago

Agree completely, it comes across that the author of the post has read some useful guidelines and created a strict rule which must not be broken.

DasIch• 11 years ago

Indeed. You should keep the complexity to a reasonable level, lines of code tends to correspond to it so it makes sense to keep the number of lines of code per function low.

The author doesn't understand the relationship, he blindly follows the advice to keep the number of lines low without considering the complexity and that leads to unnecessarily complex code.

golergka• 11 years ago

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.

therockhead2• 11 years ago

Another take on guard http://ericasadun.com/2016/01/01/another-take-on-guard/

Dylan168075• 11 years ago

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.

realo• 11 years ago

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

mhd• 11 years ago

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?

SideburnsOfDoom6• 11 years ago

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.

andrewchambers• 11 years ago

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.

gelasio• 11 years ago

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

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

Why Swift Guard Should Be Avoided

Related Stories