I tried several times to refactor a 10k+ line switch statement which had a particular case that by all logic should be unreachable, but had a comment something like this:
/* I don't know why, but if you remove this everything breaks */
I wonder if that is still there, it's been over ten years.
I got bit by something like that last year - a previous case above it had some logic without a break. I removed a case that switched on a class that no longer existed, but had a break. It was effectively stopping the fallthrough from the case that was a couple of screens above. Stuff broke, and I cursed the no-longer-employed developer that felt the need to be clever two years previous.
Java and PHP allow cases that have logic and no break/return, and it's (strongly) arguable that it's a bug. I think C# doesn't allow that. It allows fall-through, but only if the case is empty.
In C# you can use `goto' to go to any other case as required - including the one that's next, syntactically speaking. So if you need a fall-through from a non-empty case, you can do that, but you do have to be explicit about it.
It bugged me too. Here's the equivalent logic without even using all (which is definitely the right approach):
for rule in self.rules:
if not rule.canPost:
return False
return True
That's the same number of lines of code, doesn't require two imports, and is way easier to follow. I love functional programming but Python (intentionally, see reduce in py3) is not a language for doing it.
Noticed the same thing, wondered if I was missing some niche side effect here. TBH, I've noticed there's almost never a reason for a general reduce() in Python (to my usual dismay, as an ex-Clojure programmer). It does result in clearer, less clever code for the most part.
what bugs me is that his method means every single rule is always evaluated: no short-circuiting! So the code to sort the rules by weights, which is supposed to put stricter rules first to cut down on computing time, is completely useless.
Yeah i felt the same way. I can't really tell how gnarly the if statement is, but interfaces, classes, zope, xml is way worse. Every one knows how if statements work, not so much with zope xml.
Is the logic equivalent though? In the initial example, there were at least four outcomes (post, notify not a member, notify not a posting member, notify group closed) which seems too many to map on a boolean.
... into 1000-ish lines in python and some xml dialects spread over 20ish files that may or may not do the same thing... covered by 5 tests when there are clearly more then 5 initial states driving the original if....
I thought the same[1]. I've never really used any of the zope Python stuff, it seems to all be very java-esque. I guess it used to be popular (maybe still is in a niche) and if it works then great, but damn its ugly compared to more pythonic code.
Well, an if/elif/elif block that spans 300 lines is often much more difficult to comprehend than 100 - ten line functions (especially if all 100 share the same function signature).
That said, I'm not sure if I prefer the xml configuration option. But the design is overall likely more flexible now than it was before.
If the flexibility was desirable then yes but it by definition makes the code do more then it did before and is therefore harder to really understand.
In this concrete case, these many functions are now spread over the entire codebase (spanning many repos). In many classes and inheritance trees and driven by a ton of configuration so there are now many many more possibilities. Here is one implementation https://github.com/groupserver/gs.group.type.discussion/blob... which i don't find very readable personally
Reading the README in the first repo I can now see the flexibility and coupling the rules more closely to their objects was a design goal. So in fairness the new code is doing a lot more then (what i imagine to be) old code to be doing. Also I now note this code is 3+ years old so the zope dependency (which i know nothing about) makes some more sense.
That was an exciting ride! First an if statement that was longer than one would want, then he was splitting things off into small bits and a loop to make it look neat and dynamic, and then it exploded into bullshit and stopped looking like Python altogether.
That's pretty much unavoidable for complex problems.
What you want to do is make sure you don't need another 300 lines to implement that problem, or that if you do, that code is easy to read and to maintain.
Many people who don't use Java refer to Enterprise Java as the subsection of code written by people who know nothing of efficiency and maintainability within large companies.
IE: Using XMLs for things that they are not needed for.
I disagree that the problem was valid. The fact that code doesn't look right to somebody isn't a problem, it's just somebody's opinion. Beauty is in the eye of the beholder.
Wow, everything I hate about "Enterprise Software" in a single if statement. Have you ever noticed there are entire businesses built on fine-grained access control like this, that are utterly pointless bullshit? Probably full of security holes, leaky abstractions, poorly tested and responsible for creating a lot of support cases and manual intervention.
Figure out what the trust modality in your application is, and normally it's a simple graph or can be managed with a straight-forward application of world/group/user UNIX-style permissions.
Try explaining that to management types. The whole problem with "Enterprise Software" is its targeted at the manager types, you tell them they are now going to able to generate 10 reports, tell them there's an app for it the would throw money at you. They don't care about the people who has to deal with the system, they don't care it takes 10 more additional steps to do something that was really easy to do, throw in a bunch of jargon and tell them its enterprise ready and cloud based, with a touch of big data. BAM! sold!
One is unreadable garbage. (Result? WTF does that mean)? While the other is clean, maintainabe code. Anyone can tell at a glance what sin_of_degrees_times_pi_divided_by_180 holds. But result? In a function that calculates sin values? You might as well label your variables OX00001, OX000002 and so forth.
I disagree, and think people should use lots and lots of temporary variables. variables like "retval", "data" or even "result", as quoted in GP.
I have zero qualms about writing two lines
bool passes = /*stuff goes here*/
if (passes) {
}
whereas it is trivial that you could fold this into one line. the ONLY thing you're adding is the term "passes". Which most certainly is meaningful to the human reading it.
Compare:
if (!(i%2))
with
bool isodd = i%2;
if (!isodd)
which test do you think you'll accidentally flip the parity of?
Likewise I find "retval", "data", etc, to be perfectly fine variable placeholders while you treat the thing for a few lines.
In particular situations "wasted" vars like this are helpful. (although I agree with sibling comment that informatively-named functions are usually a better way to communicate meaning than extra vars) If this is your general rule, however, your code will be like twice as long as it needs to be.
Not really, sometimes you want to inspect the value that is to be returned before it is. So it makes sense to use the multi-line statements above, as otherwise everyone will change it each time.
It will also allow you to put a breakpoint on the return statement and have it print the value out or for it to be used in other debugging.
This is especially useful when the function call is used within a more complicated formula.
The idea is sound since that's exactly what rule engines are for but I feel that if you don't also post the final listing of the rules you end up with, you haven't really made the case that you improved the code.
If you want to actually solve this problem without doing the thing in the slideshow or shooting yourself, when you see
if(self.predicate_function())
foo()
else
bar()
and self.predicate_function() is only called once in the program (this is the case several times in the slideshow example), try pulling the if statement into the function (and simplify), making a
self.handle_case_something()
function instead. If the predicate is called more than once, consider subclassing. You're already doing OO, may as well do it right.
Exactly it seems like you could replace all this code with:
send("#{group.type}_notification") or its python equivalent.
Maybe a hash table with strings as keys and functions as values for a less 'scripty' language.
Perhaps I'm thinking the variable group is of type group, but it also looks like it suffers from poor separation of concerns... eg. Why are groups sending email?
Probably also a good candidate for guards instead of nested ifs
Typical case of code providing policy instead of mechanism, the solution of course being to provide policy over numerous classes, rules engines, etc instead of writing a few methods that provide mechanism.
eg. Why is this method concerned with whether the notification is email or something else...
Are persons not also the case of a group with one member?
It seems less like refactoring and more like hiding complexity in frameworks.
56 comments
[ 2.5 ms ] story [ 124 ms ] threadJava and PHP allow cases that have logic and no break/return, and it's (strongly) arguable that it's a bug. I think C# doesn't allow that. It allows fall-through, but only if the case is empty.
And because I absolutely love this article, somewhat related: http://www.stilldrinking.org/programming-sucks
Also, singleton (yuck) accessed via a helper function: yuck. Just import it.
This starts to make sense where OP professes fondness of XML: we have a Java developer!
That said, I don't understand yet why its code should be split into 9 pages of separate repositories (see https://github.com/groupserver/)...
I would have left it as the big if.
Source: https://github.com/groupserver/gs.group.member.canpost
1. https://github.com/groupserver/gs.group.member.canpost/blob/...
Well, an if/elif/elif block that spans 300 lines is often much more difficult to comprehend than 100 - ten line functions (especially if all 100 share the same function signature).
That said, I'm not sure if I prefer the xml configuration option. But the design is overall likely more flexible now than it was before.
In this concrete case, these many functions are now spread over the entire codebase (spanning many repos). In many classes and inheritance trees and driven by a ton of configuration so there are now many many more possibilities. Here is one implementation https://github.com/groupserver/gs.group.type.discussion/blob... which i don't find very readable personally
Reading the README in the first repo I can now see the flexibility and coupling the rules more closely to their objects was a design goal. So in fairness the new code is doing a lot more then (what i imagine to be) old code to be doing. Also I now note this code is 3+ years old so the zope dependency (which i know nothing about) makes some more sense.
Guess it happens.
[1] https://github.com/groupserver/gs.group.member.canpost/blob/...
What you want to do is make sure you don't need another 300 lines to implement that problem, or that if you do, that code is easy to read and to maintain.
But anyway, I see the world of enterprise Java has reached Python.
By that, you mean code that is fast, easy to maintain, typed, tested and scalable?
IE: Using XMLs for things that they are not needed for.
Figure out what the trust modality in your application is, and normally it's a simple graph or can be managed with a straight-forward application of world/group/user UNIX-style permissions.
Maybe because I stopped using variable names like "retval", "data" or even "result" - because they convey so little meaning.
How can anyone write a sin function like:
when you OBVIOUSLY should be writing: One is unreadable garbage. (Result? WTF does that mean)? While the other is clean, maintainabe code. Anyone can tell at a glance what sin_of_degrees_times_pi_divided_by_180 holds. But result? In a function that calculates sin values? You might as well label your variables OX00001, OX000002 and so forth.I have zero qualms about writing two lines
whereas it is trivial that you could fold this into one line. the ONLY thing you're adding is the term "passes". Which most certainly is meaningful to the human reading it.Compare:
with which test do you think you'll accidentally flip the parity of?Likewise I find "retval", "data", etc, to be perfectly fine variable placeholders while you treat the thing for a few lines.
It will also allow you to put a breakpoint on the return statement and have it print the value out or for it to be used in other debugging.
This is especially useful when the function call is used within a more complicated formula.
The idea is sound since that's exactly what rule engines are for but I feel that if you don't also post the final listing of the rules you end up with, you haven't really made the case that you improved the code.
Replace
withsend("#{group.type}_notification") or its python equivalent.
Maybe a hash table with strings as keys and functions as values for a less 'scripty' language.
Perhaps I'm thinking the variable group is of type group, but it also looks like it suffers from poor separation of concerns... eg. Why are groups sending email?
Probably also a good candidate for guards instead of nested ifs
eg. Why is this method concerned with whether the notification is email or something else...
Are persons not also the case of a group with one member?
It seems less like refactoring and more like hiding complexity in frameworks.
all([postAllowFilter() for postAllowFilter in filters]) Then just take a list of all of your functions.
Really, there just should have been a restructuring of the initial code flow. It didn't look too messy.