Stay Positive 2
Video transcript & code
In the last episode we simplified some code with the power of positivity. While we're thinking positive, let's have another little refactoring session.
Here's some code that was generously donated by John, of the Old Guy New Trick blog.
def disable_submit_button? return true if @file_upload_errors !zero_quantity_bids? && all_bids_confirmed? end
This is a helper method, used in display code to decide whether a submit button should be disabled.
As you can tell from some of the language used, this comes from an application which deals with bids of some kind.
What kind of bids? Who knows! All we need to understand about the context for this helper is this: it comes from a page where a user has the option of uploading a CSV file containing their bids, instead of manually entering bids through the in-page UI.
So there are two cases where the submit button should be disabled:
- If the user tried to upload a CSV file, but there was a file upload error.
- Or, if the user has already manually filled in bids correctly. In this case, that means there are no bids with a quantity of zero, and all of the bids have been confirmed.
The first line of this method is a guard clause. I'm a fan of guard clauses, but this one strikes me as slightly suspect. All of the code deals with reasons the submit button should be disabled. But one reason is handled with a guard clause, and the other is handled with boolean logic. This feels a little inconsistent.
One solution would be to incorporate the file upload error checking into the boolean expression. This requires us to do some parenthesization to keep the precedence of operations clear.
def disable_submit_button? @file_upload_errors || (!zero_quantity_bids? && all_bids_confirmed?) end
Now, do you find this logic easily understandable? If so, you're a smarter programmer than me. Mixed ands, ors, and negations make my head spin.
Let's start with the negations. The inverse of "not zero quantity bids" is that there are some zero-quantity bids. Bids with zero quantity are invalid. So the real semantic condition at issue here is whether all the bids are valid.
Let's capture that in an explaining variable.
Now one side of the "or" can read "all bids valid and all bids confirmed".
def disable_submit_button? all_bids_valid = !zero_quantity_bids? @file_upload_errors || (all_bids_valid && all_bids_confirmed?) end
Now we have one fewer negative.
Those two conditions, "all bids valid" and "all bids confirmed", seem to go together.
Lets add another explaining variable, called "all bids complete". It will combine these two conditions into one boolean value.
This enables us to get rid of of the mixed boolean "and" and "or" operators, another source of brain pain for me.
def disable_submit_button? all_bids_valid = !zero_quantity_bids? all_bids_complete = all_bids_valid && all_bids_confirmed? @file_upload_errors || all_bids_complete end
Just to make things more consistent and to capture our semantics a bit more explicitly, lets also make an explaining variable for the file upload errors.
We use the "bang bang" convention we learned about in Episode #94 to coerce the value to pure boolean.
def disable_submit_button? all_bids_valid = !zero_quantity_bids? all_bids_complete = all_bids_valid && all_bids_confirmed? failed_upload = !!@file_upload_errors failed_upload || all_bids_complete end
Now the last line of the method states that we should disable the submit button if there was a failed upload, or if all bids are valid and confirmed.
But there's still something about this method that's confusing to me. We disable the submit button if there's a failed upload… and so forth. These are still too many negatives for my tiny brain!
Let's rename the method to reverse its semantics.
And then we'll rejigger the logic to match the name. We'll start from the bottom. The submit button should be enabled so long as all uploads have succeeded, and there are still incomplete bids on the page.
The upload status is OK if there are no file upload errors.
There are incomplete bids on the page if there are either any zero-quantity bids, or if not all bids are confirmed.
def enable_submit_button? some_incomplete_bids = zero_quantity_bids? || !all_bids_confirmed? upload_ok = !@file_upload_errors upload_ok && some_incomplete_bids end
And there we have it: a more positive approach to this boolean logic. The submit button should be enabled when uploads are successful and some incomplete bids remain.
Of course, in order for this to work we'll have to either update the view code to use the new positively-named method…
…or add a wrapper for compatibility.
def disable_submit_button? !enable_submit_button? end
For extra credit, we could also factor out some of these explaining variables as explaining methods. But I think we've done enough for today. Happy hacking!