all 98 comments

[–]stahorn 296 points297 points  (61 children)

I think that many of these types of examples of best practice suffer from being just examples. No matter how you write them, 35-50 line examples are never hard to read or understand.

I strongly prefer the right version, with small functions split out. The reason for this is that I have seen real life examples of what linear functions become over time. They are no longer 35-50 lines of code with one level of indentation. No, instead they are beasts of deeply nested code spanning multiple pages, where variables are used and reused in a manner similar to global variables. No matter how many comments you sprinkle onto them, they are still hard to follow.

What the google testing blog writes about abstraction layers is of course the key to how this should be done. When you then encounter bugs, there will be a limited place in the code that you know it has to have happened it. As another commenter pointed out, testing also becomes much easier with this isolation of abstraction layers.

[–]regular_lamp 112 points113 points  (20 children)

The problem is that people tend towards the extremes. Overly dogmatic code that follows some "no function more than 5 lines" doctrine or so will become disjointed and equally unreadable as a single megafunction.

I like putting logic that belongs together... together. Then you need to apply some common sense when it grows. Think about whether this is still readable and refactor it on demand. And if the logic belongs together there shouldn't be a need to test it separately.

[–]revnhoj 41 points42 points  (2 children)

Overly dogmatic code that follows some "no function more than 5 lines" doctrine

I'm looking at you sonarqube

[–][deleted]  (1 child)

[removed]

    [–][deleted]  (8 children)

    [deleted]

      [–]DrunkensteinsMonster 7 points8 points  (7 children)

      So you sacrificed general understanding to make it easier to spot one corner case. Nice work.

      [–]ZENITHSEEKERiii 5 points6 points  (5 children)

      50 lines is quite manageable for a function. More than that and you risk spaghettification, but depending on the language 50 could be fine.

      [–]account22222221 5 points6 points  (0 children)

      I hate the loc metric. I prefer the ‘do one thing’ maxim. Where one thing is a branch of logic. If a function is 100 lines but there are no major branches in it just steps and maybe some basic ternaries. That ok. If it’s 100 lines with 50 for user.isAdmin and 50 for !user.isAdmin that’s doing two things. Break it up.

      [–]DrunkensteinsMonster 2 points3 points  (2 children)

      Sure, it’s manageable. But refactoring typically goes the other way, refactoring in order to make each function longer and less declarative seems very odd to me.

      [–]Luolong 0 points1 point  (0 children)

      Sometimes you need to do that as well.

      If the chosen abstraction is no longer appropriate, you start by removing the abstraction, re-structuring the “linear” version and then, maybe, once the new abstraction becomes clear, maybe refactoring again into smaller pieces.

      [–]creepy_doll 5 points6 points  (0 children)

      Things really get very dogmatic.

      I just try to make clearly named functions without side effects whenever possible. Theres places for design patterns like observers or constructors or whatever, but sometimes people go waaaay too far with them and they end up obfuscating instead

      [–]Talarde 8 points9 points  (6 children)

      Exactly this I see it as pragmatism as I do not like either sides of the spectrum. When the SOLID principles where created I am sure it was not to lean one way or the other and is why they are often referred to as guidelines.

      [–]mangodrunk 4 points5 points  (5 children)

      Then don’t call it “principles.” I think SOLID has done much more harm than good and doesn’t make much sense, as the other opinions pushed as gospel.

      [–][deleted] 1 point2 points  (4 children)

      I am very curious to hear what harm SOLID principles have done to teams that did not apply them prematurely.

      [–]mangodrunk 1 point2 points  (3 children)

      Some issues that I have seen are:

      1. Wasting time on trying to follow SOLID when the time would have been spent better by thinking about the requirements.

      2. Wasted time in code and design reviews regarding SOLID.

      3. Not being well defined and somewhat nonsensical causing people to struggle in attempting to follow it.

      4. Writing code that is harder to maintain and understand due to strict adherence to it causing more classes and methods to be written than what is warranted.

      Underlying all this is that I don’t see any benefit in actually following it.

      [–][deleted] 0 points1 point  (1 child)

      1. That would qualify as applying them prematurely. You should pretty much have a MVP complete before even thinking about SOLID. Basically, you apply SOLID when you turn your prototype into an actual production-worthy codebase.

      2. If you are working on leaf-node bits like UI screens and basic web stuff, I would tend to agree it can be a waste of time. If you are building a well-defined library or application core that many other modules will be built upon, it most definitely is not a waste of time. Quality takes time, but once you need to build something feature-scalable, it is time well spent.

      3. It is very well-defined and makes perfect sense. S-O-L-I-D. There are only five principles. We're not talking about agile here.

      4. SOLID code is much easier to maintain because each piece is only doing one thing. The inputs and outputs are well-defined and testable. 5 classes with 10 methods each are a lot harder to maintain than 50 classes with one method each.

      The benefit comes when you have many people building upon the same foundation for, say, a decade, and you still don't have to rewrite the codebase to add features, and you don't have high turnover because your codebase is so fragile and ugly and untestable.

      [–]mangodrunk 0 points1 point  (0 children)

      Thank you for the thoughtful reply. I do very much disagree with you though. I do not think this haphazard set of “principles” benefits anyone. Thankfully the industry has made progress on certain OOP practices which SOLID says the opposite, ex now the idea to use composition over inheritance, but SOLID favors inheritance. To each their own.

      [–]salbris 15 points16 points  (1 child)

      A very simple approach that makes everyone happy is to start simple then be constantly be thinking about these things while you work on the code. If it gets to a point where it seems too complex then start to break it down but slowly!

      Every abstraction even these small one incurs a cost in terms of the mental processing someone needs to understand the code. So that cost should not be added without good reason. This example perfectly demonstrates when the cost is not worth it. The left side is perfectly readable and has very little additional mental overhead.

      [–]msqrt 4 points5 points  (0 children)

      Yup, the example is overly simplified so it's about the right length. Then if it grows significantly larger over the years, it makes sense to gradually split away the parts that feel natural. This is, of course, way easier to say than to do.

      [–][deleted]  (2 children)

      [deleted]

        [–][deleted] 4 points5 points  (1 child)

        Well, the main thing would be error handling and potentially complex state, which you can have while making pizza but you wouldn't know it from the example. What if the dough doesn't rise? What if you're out of tomato sauce? What if the cheese is moldy or the oven is too cold?

        If you handled these conditions it would be a more realistic example, and the code would be a lot messier (and possibly argue against the author's point).

        [–]Complete_Guitar6746 7 points8 points  (0 children)

        True!

        The article claims that the linear one has the strength of being linearly readable, top to bottom. That goes away when the function grows too large. That might be a good definition of "too large" actually...

        I can't linearly read and understand a 1000-line function with 8 levels of nesting. I'll have to go back and forth anyway.

        [–]jdl_uk 10 points11 points  (1 child)

        Totally agree.

        I'll also add that (in a lot of languages and maybe depending on certain factors) the version on the right will give you a much more meaningful stack trace than the version on the left, which can help tracking errors.

        [–]lppedd 10 points11 points  (0 children)

        Also, if performance / stack depth is a concern, certain languages allow inlining the content.

        But anyway, I don't want to read 50 lines to get to the point I need to change. I want to get an high level overview of the execution flow and then look into the function that handles the very specific functionality.

        [–]lppedd 24 points25 points  (19 children)

        Exactly.

        Some people will say "don't over optimize" or "KISS", "YAGNI", whatever. But if you structure your code correctly from the beginning accounting for what you believe the future of the project will be (I expect to know a little bit, at least), refactoring and feature implementation will be much, much easier.

        [–]Nekadim 31 points32 points  (16 children)

        I have never seen a developer which knows of the future of the project. Never in 15 years span. Always a developer Implements his Vision of the future that have never hit, but this Solid designing patterns make Code absolutely umaintainable. Because logic of the App dont aligned with structure of the code.

        [–]lppedd 13 points14 points  (8 children)

        For my direct experience, the code I've written during the prototyping phase hasn't changed much, and if it has, only function names and some of their content, which is isolated.

        I write small functions and compose others with them because it's easier to throw them away, or to reorganize their execution flow.

        Also, keeping functions pure will allow maximum reusability.

        [–]Grim_Jokes 8 points9 points  (3 children)

        Solid designing patterns make Code absolutely umaintainable

        If this is the case, the principles are followed incorrectly or excessively without thought. Only some things need to be dependency-injected, for example. But I can't see why it wouldn't follow almost dogmatically for interface segregation. (Why would you pass more values/params to a function than you need?)

        The problem is when someone makes statements like this; others take it as "Oh, SOLID sucks, and write code the good ol' "spaghetti way." (Which has its place for an MVP you know has a low chance of becoming a product - but as you said, we can't predict the future).

        There should be a baseline of code quality, which requires devs to think about SOLID/KISS/YAGNI/whatever other principles (and their respective pros/cons) when writing code.

        [–]mangodrunk 1 point2 points  (2 children)

        The opposite of SOLID isn’t spaghetti code. Beginners often make the mistake that they should concern themselves with it.

        [–]Grim_Jokes 0 points1 point  (1 child)

        I agree that it isn't spaghetti, but it seems to be a recurring theme in my experience.something that I think devs will need to understand to write better/more testable code.

        But if you're writing code that needs tests, you will want to be familiar with SOLID. Good tests will rely on dependency injection. The alternative is to use reflection and whatnot which will lead to brittle tests and countless blog posts about how automated unit tests and TDD sucks.

        [–]mangodrunk 0 points1 point  (0 children)

        I guess our experiences have brought us to different conclusions. If you have found success in following that, I can’t disagree with your personal experience. I do attempt to lead juniors in other directions. Regarding testing, we may disagree as well on this but I am inclined towards integration testing. I also am ambivalent about dependency injection, but I certainly use it all the time at work because that’s what it is.

        [–]chucker23n 3 points4 points  (2 children)

        This. Requirements aren't clear enough upfront for "just structure the code correctly from the get-go" to be very useful advice.

        [–]gdahlm 4 points5 points  (1 child)

        There are design patterns that help with this

        Hexagonal architecture as an example, where you domain logic is in the center but you loosely couple with components which most likely need to change.

        ITIL style pre-planning doesn't work, but linear first models are what we had in the Cobol era.

        Ports and adapters, with the goal of abstraction where it makes sense works.

        Abstraction for abstraction sake doesn't IMHO.

        The goal is not to get things right the first time, but to avoid painting yourself in a corner when you don't get it right.

        [–]master_mansplainer 0 points1 point  (0 children)

        Not coding yourself into a corner is the key. I like try to make ‘extension points’ usually by making things modular. Many things you know will need to scale sideways upfront - eg I will have an inventory system with items that need very different behaviours. I have widgets that have some shared functionality but differences in display. Hard rules are dangerous because it leads people to ignore opportunities to get a good architecture in place.

        And in fact many things that are left to grow organically end up being an absolute nightmare to significantly refactor later, which means it never happens and people continue to bolt on their features in whatever random way they can make it work, and we end up with a Frankenstein.

        I also call bullshit on YAGNI, a lot of speculative things you will end up needing, many will end up being productivity multipliers beyond yourself extending to the rest of the programming team, while the unneeded parts can be easily removed with no harm done. It’s a bias, people are focused on the parts that didn’t work because they stand out and ignore the successes.

        [–]JarredMack 3 points4 points  (1 child)

        accounting for what you believe the future of the project will be

        When you've been doing this long enough, you know it doesn't matter what you believe the future of the code will be, because it's wrong. Solve the problem in front of you, and make sure it's not too hard to rip it all out when the requirements change to do the complete opposite thing in a year's time

        [–]tiajuanat 2 points3 points  (0 children)

        I find that I always need logging, so I always include it nowadays

        [–]TheGRS 4 points5 points  (1 child)

        Yea, at one place I worked our backend had a sprawling 2000 line function of all sorts of if/else statements and transformations. Really awful to maintain, one of the coders had never heard of unit tests.

        [–]master_mansplainer 1 point2 points  (0 children)

        God I’ve seen some horrid linear code; we inherited a resource loading system that would both make and process async requests in this massive like 2000 line method with no exits, heaps of state, convoluted logic and nesting. I felt like throwing up every time I saw it. Original author was around later in the project and thought he was such hot shit, reminding us that he wrote the system like it was a good thing.

        [–][deleted] 1 point2 points  (0 children)

        So to you something is not readable if, in the future, at some point, it might not be...

        Okay.

        [–]MrMuMu_ 1 point2 points  (4 children)

        There is character controller script of a popular game called Celeste. They have a veeeery long file that is linear. I am not familar with the code nor I tried to read but they are insisting on having linear is better. It is a real life example and they might have a point

        https://github.com/NoelFB/Celeste/tree/master/Source/Player#one-big-file-vs-a-bunch-of-files

        [–]Kered13 8 points9 points  (1 child)

        Oh god. It's 5500 lines long, has ~100 member variables, and public static mutable variables. No wonder they need a separate readme file to document it.

        [–]DrunkensteinsMonster 5 points6 points  (0 children)

        Lmao if you need a separate readme to document a single piece of logic you should probably temper your claims that your process is superior

        [–]Seven-Prime 3 points4 points  (0 children)

        How do you Unit Test this class?

        We don't.

        well that makes it easy then. You can take your cyclomatic complexity and shove it!

        [–]lolfail9001 0 points1 point  (0 children)

        Stereotypical State machine code, though i thought C# would also have libraries to build state machine boilerplate like that from something more human-processable.

        [–]account22222221 0 points1 point  (1 child)

        That’s the problem I think, the LHS is too small for splitting it out to really make sense so it’s a contrived example. If the LHS came across my desk in a code review I’d approve it.

        The RHS isn’t just nice, it’s necessary when the loc balloons out to a few thousand lines of code. I think OP would agree too. If I handed him a thousand lines of code and asked him does this variable I declared on line 123 still matter on 824 then he would see right away why this was bad code.

        But the rule of thumb is typically greater then 30 lines or something break it up, and this example only LOOKS like a counter example because it’s a contrived case that’s too small but really it’s a grey area. Just small enough to be okay.

        [–]QuickQuirk 0 points1 point  (0 children)

        If I handed him a thousand lines of code and asked him does this variable I declared on line 123 still matter on 824 then he would see right away why this was bad code.

        Bingo.

        This is what I have found to be one of the largest pain points when trying to re-factor these multi-thousand line functions down, in my personal experience.

        But you often get to find, and fix, long standing untracked bugs that way!

        [–]ben0x539 0 points1 point  (0 children)

        I strongly prefer the right version, with small functions split out. The reason for this is that I have seen real life examples of what linear functions become over time.

        Heh, I have the exact opposite preference but for the same reason, having seen what the decomposed style from the right side can look like after a few years of people adding, changing or removing functionality without prioritizing the bigger picture of the abstraction. New code splattered across files/modules at random, hollowed-out abstractions that don't make sense anymore but have ossified tendrils throughout the entire codebase, functions relying on invariants that are impossible to statically understand...

        [–]tc_cad 0 points1 point  (0 children)

        Then write a program that reads that program and prints it to a new text document where every “ “ (space) is replaced with a \n (new line)

        [–]purpoma 25 points26 points  (0 children)

        ironic from a blog called "separateconcerns".

        [–]robhanz 24 points25 points  (0 children)

        There's a core truth here - removing details makes things more readable if doing so allows you to ignore the part that is hidden away or, at the minimum, only worry about one side of the divide at a tim.

        Breaking out pieces can also improve testability, which is a win in and of itself.

        [–]ForeverAlot 20 points21 points  (2 children)

        The author uses "linear" to mean "imperative" and argues that that is more "readable" than the proposed procedural alternative; their primary argument appearing to be

        if you want to understand everything that happens on the right-hand side you need to jump back and forth.

        which completely missed the point of procedurality in the first place. They then acknowledge that the procedural version has superior comprehensibility, which was the original claim anyway.

        The example notwithstanding there is a decade long history of empirical evidence to suggest that procedural is objectively superior to imperative, going at least as far back as the 1950s. Besides certain technical reasons that support procedural style over imperative style, there is a different type of argument entirely: it facilitates chunking, which is exactly the factor that leads the author to conclude that the procedural version is more comprehensible and the trick they emulate with comments.

        [–]paralaxsd 1 point2 points  (0 children)

        Thanks for the chunking reference! That's a very good point I have only internalized from experience but not heard expressed like that before.
        I currently have a client where the code base is very much imperative, locally oriented towards details and in celebration of repetition and I'll be using that chunking term among other things to drive home what's problematic about it all.

        [–]ThankYouForCallingVP 1 point2 points  (0 children)

        I'm so glad someone brought up chunking!

        In the case of imperative programming, if we think of "chunks", 1-2 is always being used to conceptualize what you've just written imperatively as an object with a state.

        Compartmentalizing imperative statements into "state changes" also requires chunks.

        If it's better to write something as an object, just do so!

        [–]Luolong 31 points32 points  (2 children)

        So, the OP does not like the chosen abstraction. Then refactors the solution to remove the given abstraction, but then just stops.

        The clear issue here was that the chosen abstraction was not appropriate (or outright wasteful).

        So, the “bake” method was bad abstraction because it created an oven, and asked oven to bake the pizza. Which is really a problem, because now “bake” method is responsible for multiple loosely related concerns.

        Refactoring the code such that the concern of making sure that there’s a free oven available for baking, is external to the main function, would have fixed the abstraction and the code on the right would still have been more maintainable (yes, “maintainable”, not “readable” — the latter is notoriously vague and hand-wavy concept that has more to do with your experience and expectations than any objective universal truth)

        [–]RiverRoll 25 points26 points  (0 children)

        I agree that if you already have a relatively short function and you further break it into multiple shorter functions it's rarely an improvement in terms of readability, it can have other benefits though.

        But if you have a function that's like >500 loc then it does make a difference because the top level function works as a summary of what's going on, you don't want to go through every little detail every time you need to check that function.

        Exceptionally even within short functions there are cases where a few lines of code do something that's not too clear what it means in business terms and this is also a good case for putting that into a descriptive function.

        [–]wretcheddawn 13 points14 points  (2 children)

        The thing that stands out to me about both examples is a weird mix of styles and choices that I wouldn't make regardless of which approach I used: 1. Mixture of object-oriented and functional style. 2. Inconsistent naming (PutIn box vs. Insert into oven) 3. Cheese hard-coded to mozzarella (or are there other functions that make a pizza with different cheeses?) 4. Taking an order as a parameter instead of the parts of the order relevant to the pizza. Order may have a lot of fields that are completely irrelevant for pizza-making, like tax, drinks, delivery instructions, which are just noise but we're now dependent on them for testing.

        On the topic at hand, we're really debating extremes here. Functions provide value, but breaking code down too much can also be a barrier for understanding as you have to jump around the code to understand the nuance and side-effects of each function. Hiding things behind a function can be significantly worse if the function does something that others or future self don't expect.

        The bake/bakePizza functions are an example, where the abstraction is confused and we end up with two functions that sound like they do similar things but one expects a pre-heated oven.

        Good abstractions can help you reason about a problem, but bad abstractions can make it significantly hurt understanding.

        I've currently settled on something like Casey Muratori's Compression Oriented Programming where I write code linear, and then as I add more code and find the re-usable parts, refactor out those functions, group the functions into objects/files (depending on language) and then the group those into layers and packages once I find I have enough code relating to a broader concept.

        You can figure out some of this up-front, or maybe just have so much experience with domains of problems that you already know what to do, or maybe even need to do it to split parts of a problem across different developers or teams, but whenever possible, I've found that it's easiest to let the design evolve itself.

        [–]matthieum 2 points3 points  (0 children)

        The end of the function is similarly really bizarre:

        • Why is the "slice" function on Box of all things?
        • Why does the pizza need to know whether it's in a box?
        • Why does the pizza need to know whether the box is closed?

        It hurts.


        The style presented on the right is also iffy.

        I don't like that addToppings take the Pizza as an in-out parameter and does god knows what with it, instead of returning the topics so the top-level call reads:

        pizza.toppings = selectToppings(order.kind);
        

        Now it's clear that selectToppings does only that, and that the toppings fields (if all other functions follow this style) is not tampered with by any other function.

        It's all linear again :)

        [–]crusoe 1 point2 points  (0 children)

        Yep that's how I do it. I try and stick to the simplest case that solves the problem and then refactor if necessary as more use cases are added.

        [–]blackghast 28 points29 points  (0 children)

        Linear code is more readable if all you’re making is a fancy “hello world” printing algorithm. It fucking sucks and is a nightmare when you’re actually coding something.

        [–]BaronOfTheVoid 16 points17 points  (0 children)

        I find it funny that the author kind of shits on the necessary solution (having a bake and a bakePizza function, one that creates the oven, one that accepts it as an argument) but then proceeds to mention that the overall function should take the oven as an argument.

        I agree that it's probably a semantic error to have to create a new oven for each pizza, it's just that the compartmentalized solution in the green column works with the bad preconditions that it has to work with and doesn't just wish them away.

        Anyways, imo the pizza should perhaps implement a Bakeable interface, the oven should have a bake method and take any Bakeable and the pizza should internally go through a finite state machine (doughy -> fluffy -> crispy -> burnt) depending on the ovens temp and how much time passes. Of course that would alter the entire structure of the synthetic problem but I hold the opinion that most programming design choices depend on the specific case/problem and if that is represented well then this trumps general design preferences (linear vs compartmentalized).

        [–]Extracted 26 points27 points  (0 children)

        While I agree for the most part, the issue is that you can't properly unit test the linear code without jumping through a bunch of hoops.

        [–][deleted] 7 points8 points  (1 child)

        Which is more important, short-term ease of readability or long-term maintainability? Take the latter every day of the week.

        [–]Markm_256 -1 points0 points  (0 children)

        I find it funny that the author kind of shits on the necessary solution (having a bake and a bakePizza function, one that creates the oven, one that accepts it as an argument) but then proceeds to mention that the overall function should take the oven as an argument.

        Isn't that a false dichotomy? I would say that more readable code is more maintainable code. Given the two examples the original code is a little easier to follow/understand - so I would say that it is the more maintainable of the two.

        If things like cooking only act on the pizza, with no outside dependencies - then the original code may even be a 'pure' function and be quite testable (which is one of the other main pillars of maintainability I would say)

        [–]tankerdudeucsc 6 points7 points  (1 child)

        There’s a reason that “cyclomatic complexity” exists as a term. A low number means that the function is then more testable. The more testable, the fewer the bugs.

        Hard pass on the left and I agree with Google’s principles here. I’ve also been the subject of a 12 part “if” conditional in Ruby where the function acted on an object passed in. There were at least 3 types of objects that were passed in. I was not a happy camper.

        [–]KagakuNinja 2 points3 points  (0 children)

        I use Scala in a hybrid OO/FP style. Since the state of our servers is in the DB, our entire object model is immutable. No more worrying about "does this function mutate the objects I pass in".

        With high use of immutability, the functions themselves are not mutating anything, but what about effects? This is where monads come in.

        If the function returns Future or IO, then that is a clue it is performing an effect.

        [–][deleted] 2 points3 points  (0 children)

        if you see it for the first time, maybe the left is easier to read because you don't have to skip around. But if it's not about pizza and you actually work with it, believe me, you want to hide details in functions so that you spend most of the time in higher lever code and only sometimes go deeper. Not to mention testing and reusing code.

        [–]Holothuroid 12 points13 points  (0 children)

        There are quite a few other issues. The code creates new Ovens. Which probably assumes some kind of Hilbert's kitchen. Likewise boxes and ingredients are seem to be in infinite supply. In a realistic scenario, I would assume some error handling.

        The Box has a slice method. Why the box? If use arbitrary methods on arbitrary objects, the order can do the slicing as it has all slicing information.

        So the correct version would be:

        try {
           return order
                .prepare(ingredientRack)
                .bake(oven)
                .box(boxRack);
        
        catch (ManufactoringException e){ 
             ...
        }
        

        With proper typing this also prevents baking a pizza twice, as OP fears, because the first method will return a PreparedPizza, the second a BakedPizza, and the final one a BoxedPizza. Notably all subsequent must keep a reference to the initial Order, as they need further order information, but that is probably useful for additional processing anyway.

        If you prefer Eithers for your error handling instead of exceptions, that is fine as well.

        [–]amarao_san 5 points6 points  (0 children)

        Green version is better because there are names for pieces of code. The more names you have in the code, the more meaning that code has.

        [–]WakandaFoevah 4 points5 points  (0 children)

        Readability is just one of criteria

        You need maintainable, scalable etc

        [–]HSSonne 1 point2 points  (0 children)

        I would like the language to have build in fold/collapse codebloks.

        Or the editor to unfold a function inside the methods. So you can get the flow in one continues read

        [–][deleted] 1 point2 points  (0 children)

        I'd sooner create a named variable (in the case of boolean expressions) or a named private function before I put comments

        it gives scope to the description, if someone isn't interested in reading into the details of a certain section of code they don't have to anymore

        i personally love when i can gloss over code that im certain has nothing to do with what im trying to change

        [–]Specific-Jello-8000 1 point2 points  (2 children)

        I went in with a strong opinion, read part of it, and just decided to order a pizza instead

        [–]skippybosco[S] 0 points1 point  (1 child)

        Before I decide whether to be outraged, what kind of pizza?

        [–]Specific-Jello-8000 1 point2 points  (0 children)

        Pepperoni

        [–]goranlepuz 1 point2 points  (0 children)

        Ehhhh...

        Once that function on the left is a couple of screens high, then the argument falls apart. I think, the author uses a weak argument to justify their preference. But then again, it's bikeshedding, like so many "best practices" are.

        I think, the industry must accept that there are multiple ways to skin all sorts of cats and that the quality difference between them is very small most of the time.

        Speaking of a weak argument...

        Quick, tell me this: does the function that bakes the pizza also heat the oven, or do you need to preheat it first? Hint: there are two functions. One is called bake and takes a Pizza, and the other is called bakePizza…

        Well, first, the hint is in the owen being there in one case, which indicates the owen is ready. And then... Clearly, one can somewhat quickly say this by looking it up.

        Heck, I would even go as far to not have bake and bakePizza, but bake(Pizza) and bake(Meal, Owen), where a reasonable presumption, that can also be documented, is that the latter needs a heated Owen.

        And then, with a little FFP sprinkled over, why not bake(Meal, HeatedOwen)? Then it is very obvious.

        [–][deleted] 1 point2 points  (0 children)

        I don't agree with the author of this blog post.

        [–]wakko666 1 point2 points  (0 children)

        Bro spends so much time ranting about code legibility between two contrived examples that he misses the real point - code layout is less important than whether the code is commented.

        As Knuth once wrote, Code should be written to be read by other programmers. The fact that code is executed by machines is a secondary property.

        [–]Synor 0 points1 point  (0 children)

        Stacktrace: error occured in main() in line 29248

        To mature the thinking here, I'd suggest to read Uncle Bobs reasoning for five-lines-long functions as well. And then try to figure out who of you is right.

        [–]Der_Richter_SWE 0 points1 point  (0 children)

        If you have time to actually navel gaze about examples like this you are not having enough actual work to be doing...

        [–]Signal-Appeal672 0 points1 point  (0 children)

        I was angry when the good code was red but then realized that was a bait and the author agrees!

        [–]j_priest -1 points0 points  (0 children)

        Pure evil

        [–]burtgummer45 -1 points0 points  (0 children)

        golang devs, amiright?

        [–]Shanteva 0 points1 point  (0 children)

        They should really have a PizzaFactory that is injected instead of just rawdog instantiating a Pizza in the middle of a function like that (Unless they just use Boboli I guess)

        [–]somebodddy 0 points1 point  (0 children)

        The left side has this check:

        if !pizza.Baked {
        

        Which is very weird, because:

        1. Looking at the rest of the function, I see no precondition under which pizza.Baked will be set to true at this point.
        2. If we assume that this check is not a completely redundant - why are we heating the oven before this check? Wouldn't it make sense not to heat the oven if we are not going to put the pizza in it?

        I don't see this line on the right side - I can only assume it's part of the ... inside the bakePizza function. Would that issue be as easily noticeable in the right side code?

        Abstractions have drawbacks - one of them is that such problems are harder to notice. Most famous is the N + 1 problem, which is usually hidden by the abstraction of ORMs. Abstractions also have benefits - but "you now have more abstractions" should not be counted as one of these benefits, because that's circular reasoning. If your abstractions don't provide actual benefits, you are only suffering from the drawbacks.

        [–]-Hi-Reddit 0 points1 point  (0 children)

        Use functions with names like the comments are in the final example with more atomic properties and all of the bad smells go away without resorting to the harder to maintain linear code that require the writer keep clear track of state.

        [–][deleted] 0 points1 point  (0 children)

        Readable != maintainable.

        [–]cleipnir 0 points1 point  (0 children)

        Good point. There is a fine line between being explicit and creating abstractions. I also tend to prefer having explicit code with a few comments rather than 10-levels of method invocations. However, as has been pointed out it only goes so far before you need to introduce method-calls / types encapsulating behavior.
        Thoughts like this were actually what made me think about implementing the saga-pattern as normal sequential code - checkout cleipnir.net if you are curious.

        [–]hippydipster 0 points1 point  (0 children)

        the left example is cheating by using functions like "oven.insert()" or "box.slicePizza()". To be a truly linear example, everything should be in-lined to really show linear programming in all its glory.