Day 8 - Let's extract a compound conditional

1

Week two! Let’s get after it.

Today’s task: extract a compound conditional.

This is best explained in a live-coding, video format. Fortunately, I have just the thing!

Please watch the first 5 minutes of this video: https://www.youtube.com/watch?v=0rsilnpU1DU&t=13s

The linked video is a sample of a course I created recently, and it contains my best explanation of how and why to extract compound conditionals.

Again, you only need to watch until 4:58.

For those of you that truly hate video, here’s a brief text version:

Compound conditionals look like this:

if foo && bar
  ...
end

Or

if foo || bar
  ...
end

Your language might write these slightly differently, but the essential bit is that you have a conditional whose truth depends on the truth of two components combined with a boolean logic operator.

I’ve found that extracting these compound conditionals into a named method almost always improves the code.

In abstract terms, that looks like this:

Before :

if foo && bar
  ...
end

After :

if higher_level_concept?
  ...
end

def higher_level_concept?
  foo && bar
end

And here’s a concrete example:

Before :

if user_created_account_today? && user_has_unconfirmed_email?
  prevent_user_from_posting
end

After :

if user_has_high_spam_risk?
  prevent_user_from_posting
end

private

def user_has_high_spam_risk?
  user_created_account_today? && user_has_unconfirmed_email?
end

I prefer the second version because it’s more explicit. It takes an implicit idea: “users with new accounts and unconfirmed emails are more likely to post spam” and makes it explicit in the code. This is usually a big win, since it means there is more information in the code, and less that exists only in developers’ heads.

Again, there’s even more useful info in the video https://www.youtube.com/watch?v=0rsilnpU1DU&t=13s , and it’s only 5 minutes, so I suggest watching it.

Today’s challenge: find a compound conditional in your code (just grep for && and ||) and try extracting it into a well-named method. Try to make the new method explain a little more about what the compound conditional means at a higher level.

If the new name is a big enough improvement, consider committing/merging/opening a PR. If not, no worries. Just revert and remember this lesson next time you reach for && or ||.

2

Found 15 instances of compound conditions in my project. I extracted them and made them as a separate predicate method!

3

Today’s lesson is very practical. I found there are so many complex compound conditionals in my current project. I often add some comments to the conditional. That’s not enough, comment needs to be maintained as well. I will try the new approach that I learned today next time.

4

This is a really nice one. Such a small amount of work to make the code far more readable.

Thanks for the reminder!

5

A thing that I also do with complicated or compounded conditionals is to convert them to guard clauses. Sometimes within a new function

1 Like
6

This exercise made me impressed by the code base I am working on, where I have written barely any of the code.
Many compound conditionals (that were not null-checks) were defined in their own helper-method with a good name.

Will try to keep this in mind when I continue working to keep it well written :slightly_smiling_face:

7

It is nice to put a name on that kind of refactor: “extract compound conditional”, that solidifies the concept. Part of the broader concept of “making explicit what is implicit”. I mostly do this for my own code, but for the sake of today’s exercise, here is a random snippet from my codebase:

class Review < ApplicationRecord
  ...
  def valid_algolia?
    visible && content.length > 300
  end
  ...
end

I refactored thoughtlessly into

  def valid_algolia?
    visible && significant_enough_content?
  end

“Significant” is still vague and not that big an improvement. What is significance here? Low spam risk, similar the OP video? Well, not exactly in this context. I think the following hits the mark better:

  def valid_algolia?
    visible && thoughtful_review?
  end

In that we only want to surface thoughtful reviews in Algolia searches. However I would need to do more investigation before settling on a definitive name, as this is not my code. So I reverted all of this for now to stay within a reasonable allocated time, but this was a nice thought experiment. :+1:

1 Like
8

I managed to find a few places where we didn’t already have extracted more complex conditionals. It makes the code so much easier to read and understand when coming back to older parts of the code base or when new team members join :tada:

9

Found a single class that had 2-3 different ways of “compounding” the conditionals and aligned that for readability.
I personally also like to use boolean variables for readability (and to avoid generating private methods only in use in a single location).

Like so:

val userHasDoneSomething = action == something
val userHasBalance = user.balance > limit
val userCanRedeem = userHasDoneSomething && userHasBalance
if (userCanRedeem)
4 Likes
10

Good morning!

Thanks for todays challenge, I used it to clean up StimulusReflex’s codebase a bit:

If nothing else it helped me grok that part of the codebase a bit better!

1 Like
11

I got really surprised, how the readability improved. My attempt went through phases (note: I am using Python):

  1. find the compound condition (within a loop to decide, if it is the loop to write partial result out)
  2. refactor the compound condition into three functions using globals
  3. use @hauthorn “variable trick” to shring 2-line function into one line variable assignment
  4. refactor variable names to better reflect what is the reason

The code looks much better now.

Original:

    partials_written = False
    for i, (day, day_vers) in enumerate(
        groupby(versions, lambda ver: ver.last_modified.date())
    ):
        day_vers_lst = list(day_vers)
        if day > first_date:
            logger.info(f"skipping day {day} - too fresh")
            continue

        if (i % save_steps == save_steps - 1) or (not partials_written):
            version_marker = day_vers_lst[0].id
            partials_store.write(version_marker)
            partials_written = True
            logger.info(f"partials for day {i}: {day} stored ")

Refactored:

    partials_written = False
    for i, (day, day_vers) in enumerate(
        groupby(versions, lambda ver: ver.last_modified.date())
    ):
        day_vers_lst = list(day_vers)
        if day > first_date:
            logger.info(f"skipping day {day} - too fresh")
            continue

        partial_not_yet_written = not partials_written
        is_regular_partial_writing_loop = i % save_steps == save_steps - 1
        time_to_write_partial = is_regular_partial_writing_loop or partial_not_yet_written

        if time_to_write_partial:
            version_marker = day_vers_lst[0].id
            partials_store.write(version_marker)
            partials_written = True
            logger.info(f"partials for day {i}: {day} stored ")

Anyway, as this is happening within loop, I am a bit concerned about performance. If this would be speed critical case, the original was probably faster as each assignment and indirection counts to extra processing time.

I also wonder, if using “put the condition into variable assignment” the way to go (for readability reasons). Having a functions gives me the feeling (a bit false) that the condition is encapsulated and is easier to refactor.

12

This is one I usually try to be very careful with, but … as it happens, I had a couple to fix up!
Everything ends up so much nicer.

image
image1438×1572 338 KB

13

This is a very good practice, and today I have used it as an opportunity to refactor our code a little (because the first compound condition I found was in an overly-lengthy module, so I extracted some parts of it into other modules to have them more close to Single-Responsibility principle)

What I have learned during the years of programming experience is that readable code, among other traits, has this one: it never jumps through levels of abstraction. Implementation of a task with a high level of abstraction should be free of low-level details. If you need some, go down one level at a time. This way you will have a good high-level overview of how the code works, and for the details you can check the called subroutines. Sometimes this can be achieved with assigning some intermediate result to a variable just for the sake of naming it. Sometimes it is naming the compound predicates, as in this excercise.

14

Some of compound conditionals were already extracted to its methods. Thanks to the previous CQC!
And still there are a lot to do. Maybe it’s a chance to create a TODO comment? (just kidding)
Some of conditionals were removed because there was a way to combine them together like so (ruby code follows):

# before
if hash.present? && hash['key']
# after
if hash&.dig('key')
1 Like
15

Really like it, and it’s yet so simple. Managed to rewrite some if statements, definitely making this my new standard. Thx!

16

(I forgot about this challenge last week, but am hoping to join back up this week.)

I’m spending the most time at the moment working in a small Python codebase that does a lot of database work. There aren’t obvious compound conditionals, beyond somewhat standardized checks like:

if len(sys.argv) > 1 and sys.argv[1] == "stage2":

There are many compound conditionals in WHERE clauses on SQL, but I’m not sure how to extract them. SQL doesn’t have as straightforward ways to define functions, that I know of.

There is a lot of feature engineering code that does simple transforms in the python code that I’ve been meaning to extract though, and that seems closely related, so I’m going to spend some time on that. For example:

# feature: number of words in the name
df["num_words_name"] = [len(name.split(" ")) for name in df["name"]]
df["name_from_email"] = [email.split("@")[0].split(":")[-1] for email in df["email"]]
# feature: does name equal email handle?
df["is_name_equal_email"] = (df["name_from_email"] == df["name"]) * 1
17

I opted not to change anything today. Today’s task prompted a great discussion in my team about cognitive load when writing & reading software.

This kind of extraction has its time and place.

  • If the conditional were used more than once, definitely.
  • If it was an example of feature envy and hiding internal details that were leaking out into a complex conditional elsewhere, 100%
  • If the conditional is very large (>4 components)?

I’d argue changes like this can make reading for the purposes of debugging/changing harder rather than easier.

This methodology taken to extremes with many one line methods makes you jump around more, and gives you more to hold in your head over and above the data flow you’re tracing.

Others might argue that I don’t trust that methods do what their name implies and am unwilling to glaze over a method when debugging – that’s true (@jeromedalbert great point about the importance of naming).

I’m not disagreeing with @rtem that levels of abstraction are important. I love writing and encountering a high level method that makes obvious the overall flow. And I believe in strong interface boundaries. And that not mixing levels of abstraction makes things easier to change (same idea as encapsulating internal implementation, or loose coupling – fewer places touching a thing makes it easier to change).

So much of this has to do with the specific context of the conditional. In the example in the video, my team would have preferred a comment above the conditional or inline booleans (as @hauthorn suggests) rather than several new methods, or a new policy object.

There is a balance between the most procedural and the most object-oriented styles. My team sides slightly more on the procedural side as compared to today’s recommendation.

1 Like
18

:thinking: Did anyone else stop getting emails? The last one I received was Day 4.

1 Like
19

I am so glad to see this pattern here. Thanks.

20

Thanks for the write. I always write && || conditions in my code. I would like to test will it have any performance on running code to evaluate. It’s nice learning.

Also, is there are no Day6 & 7 topics? Just checking is that I missed or intended