- 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
Brillant!
Admin
TRWTF:
« {{$Errord_title = null}}
Admin
Sorry, I hadn't noticed, it actually is the title of a page ...
Admin
It looks like part of the post is missing.
What idea? It cannot be "Refactoring" as the sentence "Erika’s company wanted to implement refactoring as a policy ..." sounds funny. It feels like there was an intro paragraph about functions and methods should be simple etc, but did not make it into the final post.
Admin
tl;dr
Admin
@Nanis: in order to keep TDWTF under a maximum amount of characters - say, 1000 - the article had to be refactored (read: the other characters will be posted in another article later this day).
Admin
It was removed to make the post simple(r).
Admin
Nanis, why does it sound funny? Sounded right to me...
Admin
cornify!!!! i'm going to use this everywhere
Admin
Guy's an asshole thumbing his nose at a somewhat reasonable policy. All he had to do was put a description in his 3 updates, perhaps, Update_1_Order, Update_2_OrderItem, Update_3_AR and he get my vote for job well done.
Admin
@Fristy Taking your comment seriously, a policy would be something like "functions and methods must be short and simple". Refactoring is a method of transforming existing code to conform with the policy. Refactoring itself cannot be a policy ... It must be done towards an end.
Admin
Provided he did it like that rather than just chopping it into three pieces that are policy-compatible.
Admin
The RWTF is that his UpdateSection functions aren't 0 indexed.
Admin
Also, implementing some kind of checking if the updates have been successful or not. I guess it doesn't make too much sense to run "UpdateSection02" if "UpdateSection01" ran into an error.
But on the other hand, it's hard to tell without the actual code from the methods. Maybe he uses "throw new Exception(ex.Message);" as exceptionhandling everywhere.
Admin
I see they didn't have a policy mandating meaningful function names.
Admin
limiting the lines of code to an arbitrary number sounds like a good plan. Step 2 no class can have more than 10 properties.
Admin
LOL
Admin
My Markdown formatter refactored the first paragraph out of the article for some reason.
Admin
This policy isn't a terribly good idea, especially if you need to interface with a Database or a file.
I had to write a 300 line SQL Connection code that updated various fields in the database, I would have to open and close the Database connection 3 as the database API I was using glitches when you pass the connection object to a method.
Admin
"Refactoring itself cannot be a policy ... It must be done towards an end." - Sure it can... First commit must be the minimal amount of code, written as quickly as possible to pass a test [without breaking others]. This must be followed by one or more commits which incrementally improve the design/implementation.
Admin
"Step 2 no class can have more than 10 properties" - probably a bad policy, and one that will certainly trigger violations of the "Law or Demeter"
Admin
The thing is, while there should be useful function names and those aren't helpful, it may still make the code base easier to work with, because it will have limited the scope of any variables declared within each of the sub-functions, and when trying to establish the impact of a function you're looking at a more manageable number of lines of code. There should already be a policy requiring useful function names, so the developer would likely be called up on the naming convention they've used, but that doesn't make the policy bad.
Admin
Those three subroutines don't have any parameters. So they are independent from each other and should be in their own functions. They only should have better names.
Admin
I personally have different rule - if method is called from same class only once it should not be a separate method. I have better experience with thousands lines of code methods with well visible flow of business logic than this "cool" max 100 lines per method approach [male bovine excrement].
Admin
Or he was just fed up with retarded policies like this. The codebase could be littered with one-letter function names, global variable spaghetti code, on-error-resume-next idiocy, and God knows what else while some clown of a manager fixated on line count. If it was me and I wanted to make a point, I would've done lines 1-99 normally and then put 100-X all on the same line.
And I've been on the other end of this too, where somebody pitches a fit over a function having too few lines. Remember the "Still Empty" article from a couple of weeks ago? Try being told to do that because chaining half a dozen Replace() calls is "too complicated." There's no shortage of non-programming "programmers" sticking their noses in everybody else's business.
Admin
"thousands lines of code methods with well visible flow of business logic"
See also:
"thousands of stab wounds that ensure my continued survival and perfect physical health"
"thousands of watt speakers playing the sound of explosions 24/7 that lets me have a good night's sleep"
Admin
Who says they're independent? It could be a process that reads a file into the database (1), updates some related records based on that (2), and prints a report and cleans up files/tables (3). No parameters are required because the application knows where its SQL server is. What's to stop somebody from only calling one or two of them? Oh, we don't need a report this week, so the cleanup never happens, and next week the process silently re-updates the records from last week in addition to the new file. If the update was adding/removing inventory, now all of your inventory counts are screwed up and you have no backup from which to determine what it should really be. Imagine this goes on for months until somebody notices. Before you say it's far-fetched, I've seen this kind of stuff happen more than I care to remember. Code review doesn't catch it because 99% of code review is just busybody style policing (ie "dur hur 100 linez"). Functional details are hand-waved away as "that'll never happen, why can't you be more of a team player?" and left to testing robots that don't even test bad data, let alone logical corner cases.
Admin
Could have been worse...
Update03(Update02(Update01()));
Admin
Not.
This is the sign of someone who is lazy and stubborn. I once worked with a prima donna that when faced with a guideline of commenting code, entered "/** this is the comment */"
His code was not obvious and he basically stated that anyone who could not understand his code was stupid.
Coding standards should be guidelines, not mandates, but should be followed in general. I have refactored more than one method that was hundreds of lines to over a thousand lines.
Years ago, I found a Pascal method that was over 5K lines long with 25 nested methods.
Developers who cannot find it within themselves to write quality code should find another profession.
Admin
I asked for that to be refactored to simplify the article, so it was dropped.
Admin
In olden days when we worked on 24x80 terminals, it was a good idea to limit functions to what you could see on the screen. Shortly thereafter Microsoft started supplying WIndows example apps, where the whole 3300 line event loop was in the main function. That may be when the schism started, the Screenites versus the Stretchedites.
Admin
Suggesting that huge methods should be reviewed and possibly redone fine(the symptom of doing everything in one huge block of code is very real), setting an arbitrary limit without an exceptions is bad as shown here.
Admin
I guess I didn't accurately represent my exageration
Admin
This is not a WTF, it's a great (start of a) refactoring, since you now have three clearly independent units to work with rather than one big one. If they each had eight parameters that needed to be passed around between them, there would be more cause for concern. But now, the only thing left for Garret to do is figure out better names for the three functions.
Admin
Yeah. In general I'm much more extreme than this--100 lines is way beyond what I want to write, I generally keep it under 10. However, occasionally you get things which simply aren't subject to reasonable decomposition. They almost always fall into one of two categories:
Dispatchers. While our data is object oriented inputs from the outside are not. The longest routine I have in production code is the one that reads the config file. There's simply nothing to be gained by breaking it up. (Unless you hit what I did almost 20 years ago--the compiler choking on the routine. I had to break it into two parts and that actually made life a little harder.)
Tasks that involve many steps. Another program, ~40 lines that were just calling a whole series of steps. There was nothing meaningful to be done because they created an object and then applied a whole set of transforms and enhancements to it. One input---the configuration, one output--the resulting object. Everything else was private.
Admin
You have no idea if they're independent. You don't know what global variables, database tables, or file system objects they touch. All you know is "herp derp less than 100 lines." That's exactly why the line count mandate is stupid. Untold horrors could be hidden in those 3 functions but they'll sail right through code review that's little more than counting lines and obsessing over bracket placement.
Admin
Yep, htat was my first thought as well when I saw those 3 function names:
Oh no that bozo just added n global variables to handle state between those functions, and god help whoever accidentally references one of those global variables in another function by mistake...
Think
int days = 0; .. .. .. days = get_days(); ..
process_days(dayz);
and dayz was declared global by the bozo, and may or may not be initialised by the time it get used here.... shuder
Yours Yazeran
Plan: To go to Mars one day with a hammer.
Admin
Well, Remy said it himself... thinking is hard. So clearly the developer didn't bother.
Admin
LOC is not a proper productivity measurement, nor a proper code quality measure. Some functions need to be long. Splitting up logic into several separated logical code blocks can make code a lot harder to read in a fluent way. I've seen both ends of this shit, a 52000 lines long class with a 27000 lines long method versus a conglomerate of small classes with methods that did one tiny bit of a grander flow, spread out into several modules that was put into a call hierarchy at runtime by configuration. There were managerfactorymanagers and all kinds of masturbatory patternimplementations. And it was nigh on impossible to grok the flow after a few iterations of developing new features and branching in said flow. The bemothian mammoth class was at least fairly easy to wrap your head around if you were a bit stubborn, but the other one required you to keep a massive amount of information in your head and was easily broken. And it came to be as a rewrite of the behemoth class, and rigorously followed a "No more than 40 LOC in methods!" rule. Along with "Single return" and a few other really inane and draconian rules. For teh code qualität.
If you think fixed length rules and it's ilk belong in coding guidelines, you are doing it wrong.
Admin
Heh, reminds me ... I once worked with a small program (<200 lines) which had been ported from Occam to C -- complete with lots of indentation and no braces. Someone obviously told someone they shouldn't just dump everything in main(), so they factored out the second half of the code to a separate function, leaving the first half still just living in main().
Admin
24x80 ... mmmMMMmmm ...
Not so long ago (less than a decade) I was working at a company which was still maintaining a 30-year-old legacy written in FORTRAN on a VAX, sufficiently well-built, reliable and maintainable as to have made it non-cost-effective to have javanauted it all (the exercise to do that having been worked on piecemeal, according to available effort. Don't knock it, the strategy was sound.)
Of course, as technology marched on, so did monitors. We had a fine variety of VT emulators which worked perfectly well, allowing one the full VT experience from the comfort of a PC monitor. But these emulators were great -- they allowed you to see more lines on the screen than 24. When I first got my hands on one of these, I delightedly set it up to have far more rows than 24 -- at one time I think I even went up to 72, but realised that visibility would somewhat suffer, so I reduced it a bit -- and thenhappily went on my way with this, sharing my experience far and wide.
A decade or so after this, there was still a team maintaining this legacy code on the (as they were now) VAX emulators running on virtual machines. I had a peripheral involvement with this, being a bit of a jack-of-all-trades maintenance man. And every so often I had cause to consult with the "VAX" team. Every single one of them was still using their VT emulators in 24x80 mode.
"Why is your code all so bunched up with no whitespace? Makes it impossible to read" "Oh yeah, that's so I can get the whole thing on the screen at once." "So why don't you set your emulator to use 48 or even 60 lines?" "Um, because it's, I dunno, I got no time to fiddle around with all that! I got a job to be getting on with!"
It was no surprise that they were still maintaining the FORTRAN on the VAX and not moved forwards onto more up-to-date technology.
Admin
I would agree to this IF the update was actully broken up into three logically separate pieces. That is not always possible.
For instance, when I started programming in the 80s I was working on PL/1 mainframe code and we had a similar policy (in our case, the maximum was "one page"). So when I had to copy data from one record to another record, that had about 60 columns in it, I was technically required to split the module in pieces that were one page each. (I didn't and was never chastised for it, as I correctly interpreted the one-page rule as a guideline that didn't apply to a module of 60 similar assignments. (I also didn't follow the "one comment for each line of sourcecode" guideline for that same module.)
When a guideline such as this is in effect, and the 100-line threshold becomes more important than the idea of limiting complexity, then I would be tempted to copy Garret's approach.
Admin
Do you have some arguments or you work on strict belief system?
Admin
I have to laugh at people who have a policy "if it's only called once, it should not be a function". Functions exist to separate out bits of code into a unit; which can be reasoned about, modified, inspected, tested, etc. It does not matter whether that code is called from one place, or a thousand places. I have, many times, extracted a single line of code into a function, because then I can give that line of code a meaningful name that expresses the intention of the code, making it easier to reason about. If you look at your code and say to yourself "well, this bit should really be it's own function, but I'm pretty sure I'm only going to call it from here anyway, so leave it inline", then you're stupid.
Admin
Some days I look back on the days when I had to work with people that coded like this. Never again. I was slowly developing a bald spot from the stress of dealing with this crap 4 years ago and as soon as I got to a better company my hair grew back in.
Never ever again.
You shouldn't have to tell a dev to not have a 300 line function. They should know. They should be able to physically feel the pain associated with it. If you can't see a way to break up a 300 line function you need to seek help from another developer. 300 line sql function? I'd rather start a transaction then fire 30 async trips to the database, then if one of the trips fails I'm not stuck debugging a 300 line sql mess trying to figure out what interacted with what and where to cause the error.
Admin
If you set up your variable names correctly, you could make your 60 moves in a single statement: REC_B = REC_A, BY NAME.
Admin
Actually the event loop was normally quite short, however the WndProc was liable to sprawl. I understand that MFC was one way of alleviating the problem.
Admin
If you can succesfully chop your 300 line function into 3 bits like this without having to pass variables along from scope to scope, that means each section had entirely self-contained functionality. The only WTF here is that the programmer was unable to name his sections properly to match what the functions actually do.
Admin
I think the original poster simplified things, omitting function arguments and possibly other details. Maybe it wasn't so easy to split his 300 line function in thirds, but he suppressed the details to emphasize the main point: arbitrarily dividing up the code meets an arbitrary LOC limit, but does not make it simpler or easier to understand.
Dividing 300 lines up along meaningful lines usually does make it simpler and easier to understand. IMO, 100 lines is usually too large; it it won't print on one page, I look for a way to divide it into smaller functions, either working on different objects or collections of data, or by different major steps in working on one object. But that is a guideline, not a rule. If it was easy to split the function, I would be doing that for reasons of clarity and style regardless of the line count, and quite likely refactoring it into smaller pieces before it was even complete. But OTOH, each thing has it's irreducible complexity. Suppose I have 300 lines of nested loops, switches, and ifs. First question is whether the number of different cases does require that much logic; did I overlook simplifications and are the different cases really different? If it really is that complex, I will look for a way to break it down just so I can understand what I wrote. (Let alone when someone else has to maintain it.) So I'll look for ways to break the inner loops or or section of nested if's out into separate functions - but sometimes that requires passing a ridiculously large number of arguments, so it's better to stick with the big function, using lots of comments and whitespace to make it as clear as possible.
OTOH, sometimes there's 300 lines because you have 30 lines of structure and logic, and 270 separate data items that must each be initialized. You could split it into about 6 1-page functions, but what's the point? It's not complex or difficult to understand, just boring.
Admin
3 methods perfect!
Though to be fair I start looking for ways to break things down when I crack 10 lines in a method, unless everything in the method is related to a single operation. Even then there is often a better way...