- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
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.
Admin
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.
Admin
"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.
Admin
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...
Admin
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.
Admin
Looks like the big problem with this
AddClaimis that "Add" was taken to include "Build".Admin
It's just 'immutable' technical debt. When I squint I see a Claim object and not a method.
Admin
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.
Admin
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.
Admin
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.
Admin
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.
Admin
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.
Admin
Look carefully at the line numbers - some of the folded regions have almost 100 lines in them.
Admin
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?
Admin
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?
Admin
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.
Admin
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?
Admin
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.
Admin
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.
Admin
Are you really believing that region titles and region code match?
Admin
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.
Admin
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...
Admin
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!
Admin
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.
Admin
I thought SRP and the other SOLID principles were outdated now.
Admin
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.
Admin
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.
Admin
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".
Admin
Nonsense, SOLID isn't outdated. There are still novices who want to feel like experts, aren't there?