I agree with your sentiment @andy, I think it is best not to follow these code quality exercises blindly, but they are nice to know should one ever need them.
Sure extracting some methods and conditionals into a policy object is easier to test and technically more containerized, but now you’ve just contributed to adding a separate class and a new level of indirection. Is that really better? I’ll let you be the judge, each person has their own philosophy on the subject. I think that in many cases one can get away with a few aptly named methods (but not too many) or the inline booleans technique mentioned by @hauthorn, to keep the levels of indirection in check so the codebase can be browsed more easily. In my opinion, deciding what to do in this situation is more of a delicate balance exercise than the seemingly simple hard and fast rule that seems to be posited in today’s exercise.
This actually came up today in a PR I was working on for work. In a template, I had to style a particular HTML element with a specific class if it met two conditions. The problem was that I had to apply this style in 4 places. Started off with literally duplicating the if statement with both conditions in the four spots, which meant I had four spots that looked something like:
// check that the thing is <higher level concept>
if (object.condition1 and object.condition2)
Then realized this was a perfect opportunity to avoid the duplication, and extracted the conditions into a method on the underlying object, and gave it a meaningful name. While there’s still 4 if statements in the template, now the if statements are:
if object.some_meaningful_name()
which proved to be both a readability win (it’s more expressive), meant I could drop the superflous comment, and now if that condition changes in the future (which at this point feels fairly likely) I only have to revise the code in one place, not four.
I have created a support ticket with our email sender. According to them here are the reasons you might not receive the emails:
Their system receives a ‘bounce’ from your email provider.
You manually mark your email as SPAM
They get blocked by your spam filter
But they also gave me a pretty vague reply and stated they are looking into it. I will let you know when I have more answers. In the mean time could you use the forum threads to keep up with the challenges? So sorry about that!
Forgot to update yesterday, but was able to combine a handful of conditionals into single variables. My project is in Python a the moment, and the Ruby-esque way as shown in the description of today’s exercise doesn’t lend itself well in there. But using better variable names did improve readability still.
Agreed with some of the voices here… this one shouldn’t be taken as an absolute rule… extracting the compound conditional into helper function means that we’re adding one layer of abstraction… and too many abstraction can makes code base harder to follow…
Found an existence/nonempty condition in my Java code, and extracted it to a method on the correct object (instead of a Law of Demeter violation). A small but significant win!
I found a couple of places where step 1 had been done — extract the compound conditionals into private methods.
The intent of the private method invalid_newsletter_dispatch? was obvious, but it took me a while to figure out how the individual conditionals made the dispatch invalid. Those conditions have been extracted to methods with clear names. Next time I won’t have to figure it out again.
I’ve done this type of refactor before, but this time around led to some conversation around Ruby naming conventions—specifically, what ? should imply. In this case, I extracted a private method in a Rails controller called render_maintenance_page?, which returns a boolean based on a few criteria. A team member who’s not as versed in Ruby suggested should_render_maintenance_page?, but that doesn’t feel right to me. I’ve always understood that the ? replaces the need for is_ and the like in method names that return a boolean. It could also be that I mentally map use of should to test expectations?
@aaron Good question, most of the time I don’t use should. But I am weirdly not 100% consistent with this.
I never use should (or is) for a pure getter. However in your example render_maintenance_page? is likely not a getter just returning a @render_maintenance_page? instance variable, but it is rather a method with internal logic in it. Since it’s not a pure getter, there could be room for debate.
I will not use should for an adjective or a noun. Examples: official?, web_request?, etc.
When the method name is a verb, this is where things get dicey. With the name render_maintenance_page?, one could think that the method actually renders the maintenance page, then returns a success/failure value (unlikely and not very kosher, I know). Same for a name like redirect_to_onboarding?. Adding should_ removes any ambiguity as to whether you do a redirect or just question yourself about redirecting. I might use should in that case, if it feels right and sounds better, depending on the mood. Although I often won’t.
There are some rare edge cases where you store a boolean in some storage-like place that Rails can’t automatically map back to a method ending with ?. For example if you want to store do_xyz in Redis or an ActiveSupport::CurrentAttributes class (at the time of writing), you may want to name it should_do_xyz instead, to make it clearer that the stored thing is a boolean (and in the case of ActiveSupport::CurrentAttributes, also define a should_do_xyz? getter so the business code still gets to use ?).
This is just my personal opinion though. So in your specific example if I were reviewing a PR, I would find both should_render_maintenance_page? or render_maintenance_page? acceptable.
Found a couple of statement’s that I could extract, nothing super containing critical as I am currently halfway through a large feature, but felt good to apply the exercise anyway!
I did this exersie on 5 conditionals and especially liked that this exercise was open ended, meaning “commit it or not but keep it in mind for next time”.
I’m in the middle of a pretty big rewrite of a functionality and I’m refactoring the code around it as I go. Extracting compound conditionals feel so good. Today I’ve found a few and excercized them, but I didn’t commit. I think I’ll create a new task, maybe a wiki entry.
If feels a bit like the clean code architecture from uncle Bob where business logic is clearly segregated (just like the Policy object created in the video example).
