• (nodebb)

    Well, as a high-level function that calls other functions to get the job done, it wouldn't be so bad, but as a Big Blob O' Code that does all that itself, yeah, it's a mess.

  • (nodebb)

    I'd bet that it can't be refactored out into separate methods. Almost certainly, there are local variables declared in each region that subsequent ones depend on.

  • TheCPUWizard (unregistered)

    "you can't just refactor while working on other tickets" -- It is estimated that "drive by coding" is the root cause of over 40% of all production code failures! Laser-Focused changes (only that absolutely required to achieve the goal - minimal yet complete) are the key.

    Now as to this code... since all of the reasons are collapsed, it is impossible to see what the interrelationships are... It is possible that splitting into multiple (private) methods, each only called once could significantly increase complexity by needing to pass information between them or having extended lifetime by using enclosing scopes.

  • Tinkle (unregistered)

    That code is possible without regions, and then it is even worse. Regions at least 'documented' it.

    When a method starts to reach more than a full screen then it is time to consider splitting it.

    If only there was an easy way to select the code, right-click it and have a menu pop up that lets you extract it as a method or local function...

  • (nodebb) in reply to colejohnson66
    I'd bet that it can't be refactored out into separate methods. Almost certainly, there are local variables declared in each region that subsequent ones depend on.

    In that case, it can be refactored, but the result might be even messier than having it as a single function. You would either end up with functions that have a dozen parameters and return a dozen values, or you'd have a dozen extra object properties that don't need to exist outside AddClaim.

    The level of WTF in this one definitely depends on the details.

  • (nodebb)

    Looks like the big problem with this AddClaim is that "Add" was taken to include "Build".

  • fritz crackers (unregistered)

    It's just 'immutable' technical debt. When I squint I see a Claim object and not a method.

  • (nodebb)

    For those talking about this would be more of a mess if you put those regions in another functions, who's to say that it's not already like that? Each region might be calling another function to do the work, and doing stuff with the return value that can't be put in those functions, as every time those functions are used, the return variable is handled differently, and at bare minimum, the submit claim part either is the heart of this function (like calling multiple insert queries, at least 8 of them) or is yet another function call to a submit claim function with at least 10 parameters. As we don't see the inside of the region, we can judge the WTF fully.

    The one I caught that no one seems to have mentioned though... the AddClaim function itself is wrapping in a region. So you can fold on either the function itself, or the region to collapse the same amount of code. Unless this is some odd C# only thing because editors didn't support folding on the functions themselves at some point when people first learned it.

  • (nodebb)

    The worst function I ever saw was a 14k-line main event loop (in an old MacOS 7-based app). It was half of the entire application, which was all in a single .c file.

  • (nodebb) in reply to colejohnson66

    It can still be refactored, it just might take a bit of extra work. At the extreme, you can define a new struct/object which is only used within this workflow where you can attach the results of different steps (basically just glue the local variables into it) and pass that object to the different functions you've refactored it into.

  • AgRi (unregistered) in reply to miquelfire

    Looking at the jumps in line numbers for each region, I think it's fairly safe to assume that this is in fact a Big Blob O' Code as Steve_The_Cynic called it. For instance, the region "Claim and Claimant Details" spans 100 lines, "Medical Details" spans 90 lines, and the most egregious seems to be "Accident Details", which spans 160 lines. I would certainly hope that in all those lines there isn't also a call out to a separate function.

  • geoso (unregistered)

    An insurance company, and only 700~ lines to add a claim (i.e. half of their core business actions)?

    That's pretty efficient, all things considered.

  • (nodebb) in reply to miquelfire

    For those talking about this would be more of a mess if you put those regions in another functions, who's to say that it's not already like that? Each region might be calling another function to do the work

    Look carefully at the line numbers - some of the folded regions have almost 100 lines in them.

  • Sauron (unregistered)

    any refactoring efforts need to be budgeted for

    If they love budgeting, then wouldn't it be possible to tell them that an estimated 42% needs to be added to the budget of every non-trivial ticket, because of the interests to pay on the massive technical debt?

  • Argle (unregistered) in reply to mynameishidden

    The book I used to learn Windows programming (back in 3.00 days) showed all its examples as humongous switch statements. Page after page of code all in a single function. Is it any wonder we turn out bad programmers when we teach them bad techniques?

  • (nodebb)

    Ironically that's exactly what DDD domain services look like - the end up being god objects with dozens of smaller services getting injected. I guess the developer skipped the injection part, but it hard to tell what is actually IN the regions because they are collapsed. Could be just a few lines each setting up and calling the corresponding injected domain service.

    Besides, nothing is worst than having 100 methods for a method that would be 50 lines. More methods doesn't increase readability, it actually greatly reduces it when overdone. Because then you end up either endless method names or names with so little information you have to jump to a different block in sources code. And that's a flow interruption and that's always bad for readability. So there's no rule of thumb, it's more an art form and to get it right usually takes a few passes of refactoring.

  • gman003 (unregistered)

    Hot take, I actually prefer big functions like this, over a function that breaks things up into sub-functions that only ever get called from that formerly-big function. Sometimes, a function just has to do a lot of things, and I'd rather see them in the order they happen, than have to keep bouncing between that parent function and the ones that do the work, to check what order they happen in and any other little work that gets done in the parent. I even use unnamed { } blocks as a code organization method, much like these #regions.

    Part of this may be due to having been bitten by a nasty bug early in my career, where I followed Best Practices and broke a big function up into smaller functions, and then someone else (one of my seniors, actually) wrote code that called one of those subfunctions directly, thinking it was instead the main big function. It did just enough of what they wanted to pass tests, and then caused major problems in production. Debugging it was a nightmare until I realized it just never called the main function. And sure, maybe the naming could have been clearer, or maybe this would have been trivial if this project had a proper debugger. But I've found it's also just harder to come back and understand code, when it's needlessly broken up. Code that runs linearly should be read linearly.

    So my rule of thumb now is not "is this function over N lines of code" or "can this function be broken up into smaller steps", it's "will any of those functions ever be useful to call from anywhere else". Sure, maybe the transaction processor would be shorter to read if the "lookup accounts", "check business rules", "log transaction", and "update balances" were separate functions, but do you really want to make it easier for someone to update balances without also logging the transaction?

  • (nodebb)

    The problem here is that this isn't "AddClaim", but "CreateClaim". We have sections for each of the types of information needed. Perhaps some should be refactored out but if all you're doing is auto claims it probably doesn't matter.

  • (nodebb)

    The real WTF (in a good way) is that someone is using the #region tags to put useful comments into the code about what each section of the code is supposed to be doing. If you have to deal with a "god function" it's better to have a map to what it does than to just be staring at a tangled mess of code that the author thought was obviously self-documenting.

  • Álvaro González (github)

    Are you really believing that region titles and region code match?

  • (nodebb)

    If it weren't for the line numbers, I wouldn't be surprised if each region didn't just contain one or two lines of code.

  • (nodebb) in reply to gman003

    I tend to agree with you on this.

    I will often prefer bigger functions if what is inside the function will never need to be called independently from somewhere else.

    Break away the code that can be useful elsewhere in the system to its own function.

    So something like 'Lookup Account' would be great as its own function, as you will want to use it elsewhere, not just 'addClaim' but 'lookupClaim', etc...

  • Anon (unregistered) in reply to gman003

    Not sure what language you were working in, but critical for this kind of design is making the "helper" functions private and exposing only a limited interface. Makes it effectively function like a big blob from the outside but helps readability from the inside

  • Your Name (unregistered)

    I'm totally with gman003 on this. Logic that belongs together needs to be together. Never break up the code flow or debugging flow just for some random guide line. Arbitrary metrics religiously followed like "> xx lines is too big" are academic rules for stupid people.

    Break out into several functions if the code is reused. If the code in the functions needs to be called in order, breaking it into functions makes it possible to mess up that order. Rules people would probably add some state to the code enforcing the order then (and further convolute it).

  • (nodebb)

    Pfft! You and your thing against regions! Regions are totally fine - says my coordinator - our was-considered-best employee uses them abundantly and you know what? They even have got some new support in VS Code minimap! So they must be good, used and current practice!

  • Drak (unregistered) in reply to gman003

    I totally agree with you. I have some co-workers who split everything up into a bazillion little functions and trying to follow the code is a nightmare. Especially when doing code reviews and you are literally jumping all over the place to see what happens with the result of one tiny function which is passed to ten other functions, but because of the code change it can now be 'null', so you have to jump to each of the functions to see if they can handle that, instead of just reading on downward to the end of a bigger function.

  • Officer Johnny Holzkopf (unregistered) in reply to Tinkle

    If interpreting how source code takes up screen space as a metric to decide if to consider splitting methods, people will find interesting ways to "game" that metric: Physically rotating the (common widescreen) monitor to portrait mode gives you more Y than X, and today's screens are quite big and have high resolution. Then switch to an ant-tiny font, and suddenly it is no problem to have methods containing several 100 LOC and say, "See? Fits on one screen, no need to split that code!"

  • ichbinkeinroboter (unregistered) in reply to TheCPUWizard

    I have been bitten by this "bugs from refactoring" myself. And from colleagues (something as innocuous-seeming as a change to destructured parameters in js can wreak havoc!)

    I tend to be keen to refector.

    My current teammate is of your mind.

    Pragmatically, I think the art is in deciding when it makes sense to split down (easier testing, actual [NOT THEORETICAL] reuse, other good reasons). But yui need solid set of regression tests or it reall yis better to leave well (ill? ) alone... I go by a "rule of 3" - no abstractionuntil you have definitely 3 real case that use the code to be abstracted.

  • ichbinkeinroboter (unregistered) in reply to Drak

    IDE handles the jumps though, with a keyboard shortcut surely?

    Though my teammate is of your mind .... and KNOW he uses vim, so I get it.

    Personally I hate reading a function that is more than one screenful. But that is my weird eyesight maybe (longsighted, old, less I move my eyes the better)

  • (nodebb)

    I thought SRP and the other SOLID principles were outdated now.

  • (nodebb) in reply to TheCPUWizard

    The number of times I've gotten reprimanded for "why did you format that block of code you were working on? Now it's impossible to tell what was really changed in the pull request" makes me pull my hair out.

  • (nodebb) in reply to gman003

    We have an internal app at work that was designed this way, because that's how it logically works. It does actions on things, multiples sometimes in precise steps. It' more or less procedural code because that's how it behaves. Breaking it apart into multiple classes/methods would just create a lot of overhead and confuse people while reading code top-down that processes in a linear way is fine.

    Is it fine for every scenario? Of course not.

  • commenter (unregistered)

    @DocMonster, why not just make two pull requests? Or better, just don't change the indentation just because a branch has been added. Turning on "ignore whitespace changes" makes it impossible to detect inconsistent indentation and wastes CPU cycles.

  • Kriis (unregistered)

    Regions and large blocks of code aren't, in of themselves, a bad thing - just like Goto isn't evil. They can be indicators of "bad things" but if a block of code is long? So? If it does the job its supposed to and - per the comment on the page - has been in production for years?

    Sounds like the definition of good code to me.

    The only thing worse than bad code... is strict adherence to "rules" like SRP or "Don't use GOTO".

  • Tony (unregistered)

    That's why many languages have sub-functions.. the problem isn't that a function is 1000 lines, it's what when it does multiple things like that a change at the top can change something at the bottom as there's no isolation and you can't just test part of it.

    There are far worse things to deal with though, and at least adding regions has labelled the different purposes.

  • Oracle (unregistered)

    Only 772 lines? That would be a short and coherent function in my current project.

  • Duke of New York (unregistered) in reply to DocMonster

    Nonsense, SOLID isn't outdated. There are still novices who want to feel like experts, aren't there?

Leave a comment on “Regional Variation”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article