Day 8 - 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

This is a very ticky one for database guys for we don’t have the tools of you OO-guys - especially when it comes to SQL queries.
As for oracle, we have user-defined functions we could use to abstract some conditional logic, but it can have serious performance-implications (due to SQL/PLSQL context switching).
I therefore used the time to read a bit about the possibilities (which consumed much more than 20 minutes) and trying to include it into a current feature I am working on.
My findings so far (on an Oracle 12.1, might be even better on newer oracle versions) on the following use-case:

We have several rows of projects and want to show whether a project is finished or not. A project is either finished if it’s canceled (IS_CANCELED) or if the year of a specific date (VN_DATUM) is same or lower then the current financial year.

Common approach would look like this:

select
    case
      when is_canceled = 1
        or ( vn_datum is not null and extract(year from vn_datum) <= fin_year.year) then
        1
      else
        0
    end is_finished
  from my_projects
    cross join (select get_cur_fin_year() year from dual) fin_year

get_cur_fin_year() is a function with PL/SQL result-cache enabled.

I extracted the condition into a package which looks like this:

create or replace package body project_util as

  function date_reached_fin_year( i_date in date )
    return boolean
  as
    begin
      if ( i_date is not null
           and extract(year from i_date) <= get_cur_fin_year() )
      then
        return true;
      else
        return false;
      end if;
    end;

  function is_finished(
    i_is_canceled in my_projects.is_canceled%type,
    i_vn_datum in my_projects.vn_datum%type )
    return integer
  as
    begin
      if ( i_is_canceled = 1 or date_reached_fin_year(i_vn_datum)) then
        return 1;
      end if;

      return 0;
    end;

end;
/

So the select now looks like

select
   project_util.is_finished(is_canceled, vn_datum) as is_finished
from my_projects

The performance of the latter was about 30% slower, which is absolutely okay in my use-case (only several hundred or thousand projects). I tested with 10’000, 100’000 and 1’000’000 test-entries. Significant performance issues only came with the 1-million test. Even with 100’000 test-entries the performance was ~280ms vs ~360ms - hardly noticable.
Hint: Adding pragma udf to the function did only increase performance by a little bit, though I think that’s due to the bit outdated oracle-version.

Edit: Adding result_cache to the is_finished-function decreased performance terribly. deterministic had no significant effect (this is highly dependent on the data stored - in my case the VN_DATUM is different for each project)

1 Like
3

I love this kind of improvements. I’m of the philosophy that code is for humans to read and computers to incidentally execute.

I converted this:

for (Screen screen : Screen.getScreens()) {
  Rectangle2D bounds = screen.getVisualBounds();
  if (stage.getX() + stage.getWidth() - MINIMUM_VISIBLE_WIDTH >= bounds.getMinX() &&
      stage.getX() + MINIMUM_VISIBLE_WIDTH <= bounds.getMaxX() &&
      bounds.getMinY() <= stage.getY() && // We want the title bar to always be visible.
      stage.getY() + MINIMUM_VISIBLE_HEIGHT <= bounds.getMaxY()) {
      return false;
  }
}

into this:

for (Screen screen : Screen.getScreens()) {
  if (isStageWithinScreen(stage, screen)) {
    return false;
  }
}

So much more readable! The extra method looks like this:

private boolean isStageWithinScreen(Stage stage, Screen screen) {
    return stage.getX() + stage.getWidth() - MINIMUM_VISIBLE_WIDTH >= screen.getVisualBounds().getMinX() &&
            stage.getX() + MINIMUM_VISIBLE_WIDTH <= screen.getVisualBounds().getMaxX() &&
            screen.getVisualBounds().getMinY() <= stage.getY() && // We want the title bar to always be visible.
            stage.getY() + MINIMUM_VISIBLE_HEIGHT <= screen.getVisualBounds().getMaxY();
}
1 Like
4

Tricky in certain PHP situations - especially when working with data POST-ed to the server. To avoid warnings, we need to check if $_POST['fieldname'] is set before trying to access it, so we have a lot of conditions like this: if(isset($_POST['fieldname']) && {check the actual thing we want to check}) (or at least I do).

Maybe I could wrap up all $_POST access in an object, (and maybe I should), but that would just push the compound conditional down a layer or 3.

In most other situations, like those in the video, this works for us.

I went from this:

if(strtoupper($this->getData('country')) != 'CA' && strtoupper($this->getData('country')) != 'US') {
// ...
}

to this:

if( !in_array( strtoupper($this->getData('country')), ['CA','US']) ) {
// ...
}

and finally to this:

if( !$this->hasValidCountry() ) {
// ...
}

///===
public function hasValidCountry() {
	return in_array( strtoupper($this->getData('country')), ['CA','US'] );
}

And did the same thing for a few other conditions.

1 Like
5

In my VueJS app one part of the site requires hiding the navbar, so I just did:

<div class="navbar" v-if="$route.name !== 'hiddenroute'">

But then other routes needed to hide the navbar, so it became

<div class="navbar" v-if="$route.name !== 'hiddenroute' && $route.name !== 'hiddenroute2'">

And then later there was a free trial banner added which also shouldn’t be shown on those pages, but only if the user is not active, so we got

<div class="trial-banner" v-if="isActive && $route.name !== 'hiddenroute' && $route.name !== 'hiddenroute2'">

Having both of those hardcoded in the app is not going to be sustainable and so the refactoring became:

<div class="trial-banner" v-if="showTrialBanner"/>
<div class="navbar" v-if="showUI"/>

showUI() {
    return !routesWhichHideUI.includes(this.$route.name);
},
showTrialBanner() {
    return !this.isActive && this.showUI();
},

And this is even the start of a nice little access control list system. I’ll call this a nice little improvement.

1 Like
6

Made a minor change to one of our internal repos where it was checking the status code of an HTTP response. The gist was the code was previously like:

if response.status == 200 or response.status == 204...

and I refactored to:

if successful_response(response):
....

def successful_response(response):
    return response.status in {200, 204....}

Small win. As part of doing this though I wanted to look at the test coverage report to make sure the code I was changing was covered by a unit test. As it turned out the CI server for this project was configured such that while the test coverage report was generated, it was thrown away after building so I fixed that so it archives the test coverage report so we can actually look at the test coverage for a given build. Nice little win!

1 Like
7

This conditional existed a bunch of times in one procedure, so a small win. Since only one procedure was affected, extracted the compound conditional to a private function within the procedure, so no implications outside of the procedure. Changed this:

IF lv_event IN (c_event_OfferDeclined, c_event_OfferAccepted)
        THEN...

Into this:

FUNCTION f_is_offer_event
        RETURN BOOLEAN
    IS
    BEGIN
        RETURN lv_event IN (c_event_OfferDeclined, c_event_OfferAccepted);
    END;

IF f_is_offer_event
        THEN...
8

Heh… did a grep for if.*&& and opened the first file it found… and the compound conditional I extracted even had a bug in it! https://github.com/WikiEducationFoundation/WikiEduDashboard/commit/52a70040d689a04d66ddb2e1ff17f80acfc44035

1 Like
9

I got to use this in the process of fixing a bug today:

   def import_disabled?
 -    import_type == 'delta' &&
 -      args[:started_by] == PERIODIC_RUNNER_IDENTIFIER &&
 -      !FeatureFlags.active?(:run_periodic_delta_imports)
 +    [
 +      import_is_delta?,
 +      import_is_from_cron?,
 +      !delta_import_active?
 +    ].all?
 +  end
 +
 +  def import_is_delta?
 +    String(import_type) == 'delta'
 +  end
 +
 +  def import_is_from_cron?
 +    args[:started_by] == PERIODIC_RUNNER_IDENTIFIER
 +  end
 +
 +  def delta_import_active?
 +    FeatureFlags.active?(:run_periodic_delta_imports)
    end

I’m not thrilled about expanding the number of lines, but I think it makes the intentions a lot clearer. The bug was that i couldn’t count on import_type being a string, so I’m forcing a conversion.

10

resource.uid == resource_params[:uid] => resource_matches_requested_uid?