Re^6: The Boy Scout Rule

Example 1

Can you imagine the interview? "What data structure would you use to store 1 to 10 values of the same type?" — "10 variables?" — "Correct!"

Yes, that is horrible. But ...

Does it work?
However clumsy this solution to problem is, if the system is running and has been for a while, one must assume that it does.
Is it clear what it does and how it does it?
Again, it is a clumsy way of doing it, but it is pretty obvious how it does it.
Programmers may be offended by it, but they should not have any trouble understanding it.
And from the business persective it is working code contributing to the revenue stream.
And users will be completely oblivious to the horrors.
Is there a history of errors requiring fixes for this piece of code?
I don't have access to the modification history; but cursory inspection of the style and layout suggest that this was probably all written by the same author at the same time.
It is possible that extra layers have been added, to accommodate expansion of the permission types etc.; and that could hold a clue as to how it ended up like this.
Perhaps it started out as just one or two levels deep and was essentially okay; but because of the style used for the original implementation -- the use of what appear to be globally scoped scalars to carry state -- the simplest way to extend the permissions was to stick with the existing mechanism and just add levels of nesting; because to do otherwise would incur knock on changes affecting huge amounts of the codebase.
Beyond the generic -- better code is just better -- is there anything specific that would be improved by refactoring this piece of code?
How many other pieces of code would require changing in order to allow this piece of code to be cleaned up?
As offensive as the use of global state may be to modern programmers used to languages that provide for modularisation; limited scoping and data hiding; there are thousands of programs in other languages -- COBOL & Fortran to name but two -- still running, that use that style to process huge amounts of data every second of every day.
As offended as you might be by them; those programs were often 'state of the art' when they were first constructed. And they don't stop working because ideas about program construction have changed.
Unless you can find some measurable, concrete benefit to the bottom line to justify changing the code -- and changes here will inevitably require big changes in many other pieces of code -- that adds up to a huge risk for no measurable reward.
That is to say, if you implemented the change to put 10 values into an array, made the required changes every where else in the code base that would be affected and got it right first time; what you end up with is a working system that is fractionally cleaner; but will not improve the customer experience of bring in more revenue.
Unless you can find that measurable benefit from the improvement; the risks far outweigh the benefit of "cleaner code".

Example 2

Essentially, exactly the same as above.

Whilst not pretty, it is pretty simple and clear.

Unless there are improvements that are entirely localised to that subroutine, which given the number of external (presumably global) variables being referenced and changed, would probably be very superficial, then you have to look at the affect of any changes here on the rest of the code base and do the risk .v. reward calculations.

Which on cursory inspection do not look good.

I suspect that the only way to tackle refactoring this codebase would be to start by converting the global state into a god object and passing that around to every subroutine. Once you've done that, you can start breaking the god object into sub-objects that only get used by subsets of the functions; and then slowly work them into separate modules or classes.

I've done that a few times and it is a pretty thankless task. In the interim you break things a lot; and even once you've got it all working again, it often runs measurably more slowly and often requires more memory, and all you've done is get back to where you started in terms of functionality.

Of course, the hope is that future additions and maintenance are easier; but that only becomes measurable in the future.

Get it wrong and you're the guy that broke everything.

With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

"Science is about questioning the status quo. Questioning authority". I'm with torvalds on this

In the absence of evidence, opinion is indistinguishable from prejudice. Agile (and TDD) debunked

Comment on Re^6: The Boy Scout Rule

Replies are listed 'Best First'.
Re^7: The Boy Scout Rule by choroba (Cardinal) on Jan 27, 2015 at 18:03 UTC
Whilst not pretty, it is pretty simple and clear. I beg to disagree. I recently implemented new features that needed to exit, but I had to literally experiment with all the possible combinations of exit statuses and AndExits to find the values leading to the expected behaviour. There's no documentation and only the comments shown. I'd think that "and exit" means "exit", not "and exit if not called from the command line and the number of errors is greater than some arbitrary constant plus a global variable whose value changes in 10 more places, good luck". Otherwise, you're right. The global state is enormously large and contains lots of magical hashrefs as the one shown in Can you use string as a HASH ref while "strict refs" in use?. Also, many subroutines start with a boilerplate of `my ($i,$j,$k,$l,$m,$n); my ($Results,@Results,%Results);` [download] You know, just in case you needed a variable, but couldn't declare one. One or two of them are actually used sometimes, but you never know which ones. لսႽ† ᥲᥒ⚪⟊Ⴙᘓᖇ Ꮅᘓᖇ⎱ Ⴙᥲ𝇋ƙᘓᖇ	[reply] [d/l]
Re^8: The Boy Scout Rule by BrowserUk (Patriarch) on Jan 27, 2015 at 20:36 UTC
I beg to disagree. I recently implemented new features that needed to exit, but I had to literally experiment with all the possible combinations of exit statuses and AndExits to find the values leading to the expected behaviour. I meant the code rather than the logic. And I did say my inspection was cursory. (After a second, equally cursory inspection, it looks like the (il)logic is all there; but it would need consentration or a paper walk through (or experimentation) to tease it out.) However, the need to add additional functionality sounds like the ideal time to refactor the internal logic of the subroutine. Or at the very least, an ideal opportunity to at least document it a little. the one shown in Can you use string as a HASH ref while "strict refs" in use?. That's a doosey. But ... (you knew that was coming right :), I suspect that the technique used was probably quite common (and perfectly normal) back in the Perl 4 days. (Note: I never used Perl 4; but I do remember seeing a very early version of Shell written by Larry Wall, that used techniques that today we would baulk at completely, but ...) All said and done, when you use a package global, all you are doing is adding a key into a hash (the stash or symbol table hash); and all that code is doing is using the value held in one variable as the key in that hash. Today this practice is condemned as Why it's stupid to `use a variable as a variable name', but the suggested replacement for this -- using the variable as the key into a hash -- is actually exactly what is being done in the condemned practice, with the only difference being the visibility of the hash in question. The replacement, using a lexical hash, thus contained scope, is better; but before lexicals were available, a lot of perfectly functional -- and assuming familiarity with the techniques used then -- perfectly maintainable, code was written that way. Also, many subroutines start with a boilerplate of `my ($i,$j,$k,$l,$m,$n); my ($Results,@Results,%Results);` As those are lexicals and contained within subroutine scopes, it seems to me they are a low-risk, no-brainer, target for cleanup. And if the management cannot be persuaded of that, then I think I would be looking for employment elsewhere also. With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday' Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error. "Science is about questioning the status quo. Questioning authority". I'm with torvalds on this In the absence of evidence, opinion is indistinguishable from prejudice. Agile (and TDD) debunked	[reply] [d/l]