Put conditional logic inside method for DRY, or keep it outside for readability?

Question

Take the following example which loads an interstitial ad every 10 times user does XYZ in the app, under certain conditions. It is called in multiple places in the code base:

public class AdHandler {

    public void showInterstitialAd() {

        if (Subscription.isSubscriptionActive()) {
            Log.i(TAG, "Subscription active, not loading interstitial ad");
            return;
        }

        if (!EventTracker.isEventThresholdMet()) {
            Log.i(TAG, "Threshold not yet met, not loading interstitial ad");
            return;
        }

        ... potentially more conditionals that return early if not met

        // Finally create and show the interstitial ad because the conditional checks have passed
    }
}

In this version, all of the logic of whether or not to actually load the ad is inside the method that loads the ad, which seems logical. And assuming that I call showInterstitialAd() in multiple places in the code base, it's DRY as I'm not checking isSubscriptionActive() and isEventThresholdMet() in multiple places (and there could very well be more conditions/logic to evaluate).

My one concern is that from the outside perspective, without actually looking at showInterstitialAd()'s declaration, any call to showInterstitialAd() looks like the ad is being loaded every time, with no conditions around the loading. While that's not the worst thing, one can imagine that having 50+ methods that follow this declare-conditional-logic-inside paradigm can make the code base a bit convoluted at first glance, and hard to follow without having those method definitions open all the time for reference.

If we look at the other version, it would be something like this:

public void firstMethod() {

    ... more code above

    if (!isSubscriptionActive() && isEventThresholdMet()) {
        showInterstitialAd();
    }
}

public void secondMethod() {

    if (!isSubscriptionActive() && isEventThresholdMet()) {
        showInterstitialAd();
    }

    ... more code below
}

This version makes it obvious what conditions cause the code to run, but is also a major cause of duplication of the conditional logic. My gut says version 1 is more maintainable in the long term, especially if showInterstitialAd() is called in many places, even though version 2 looks more readable at first glance.

Is there some advice/reasoning from more senior people, or books like Clean Code that can give some reasons why one approach is better than the other? I'm specifically looking for reasoning in regards to readability and maintainability.

Answer 1

I think that the logic in the existing code base is fine. I think that the method name is the issue.

Instead of naming it showInterstatialAd, why not name it something that tells what the method does, like attemptToShowInterstatialAd (or something less awkwardly worded with the same idea)?

Answer 2

I agree that option #2 would be a violation of the DRY principle (especially if strewn across hundreds of classes), but in option #1 the function lies - or at least fibs - about what it does: It does not actually always show an Interstitial Ad.

While I agree with CandiedOrange that names like showInterstitialAdMaybe() are meaningless noise (what does maybe mean? when the stars are right?) it does not mean that you should omit the rather-important fact that it does not always show the ad. You need a better function name.

Perhaps split showInterstitialAd() into two functions: One that does the ad-showing (always!) called... showInterstitialAd() and one that determines if ads should be shown before calling showInterstitialAd(). Perhaps something along the lines of showInterstitialAdIfNecessary() with a detailed comment what necessary means in the function's description.

Answer 3

My one concern is that from the outside perspective, without actually looking at showInterstitialAd()'s declaration, any call to showInterstitialAd() looks like the ad is being loaded every time, with no conditions around the loading.

No it doesn't.

I really can't stress this enough. You have no right to assume that any method named showInterstitialAd() actually does what it says it does. Why? Abstraction. Whatever implementation you're talking to has the right to decide how to respond.

If doing nothing isn't a failure it has no obligation to do anything. You can see this idea in the Null Object Pattern.

When you call showInterstitialAd() you are issuing a command not announcing an event.

If the name had been interstitialAdShown() Then, by damn, it better have been successfully shown. But showInterstitialAd() is a command. Commands can be rejected. They can be rejected with exceptions, error messages, or simply by doing nothing at all.

In the object world showInterstitialAdd() is a message. Exactly what the response to that message is not something you get to know from the name. If you did then the implementation details are leaking out of this abstraction.

So names like showInterstitialAddMaybe() are adding meaningless noise. You should already know that you haven't been guaranteed anything will happen. Only what message will be sent.

Imagine I tell you to jump and you ignore me. Does it mean I got the name of the command wrong or does it mean I'm not your drill sergeant? Suppose by design you only jump when told on Saturdays. Do you owe anyone an explanation of why you're your not jumping on Tuesday? Should I have to understand your Saturday rule to use you without you throwing exceptions at me? Abstraction and polymorphism only work when you don't require your clients to understand your implementation details. So stop thinking the name tells the whole story.

Examples include names like open() not openIfFileExistsOtherwiseThrow() and System.out.println() not System.out.printlnButOnlyIfOutIsNotDisablingPrinting()

Stop letting implementation details leak into your names. Names should convey intent, not results.

If you do need to show the conditional nature at the current level of abstraction the only excuse is if you want to show WHICH conditionals are relevant.

if ( isEventThresholdMet() ) { 
    showInterstitialAdToGuests()
}

That tells me so much more than meaningless warts like "maybe" that only remind me that it might do nothing. Every command might do nothing.

Answer 4

!isEventThresholdMet() is wrong.

If something is an event, then nothing elsewhere in the code should check if the event should already have been fired.

You should fire an event when the threshold is met and thereby call the handlers for that event.

That is the way to handle events, doing anything else leads to strange code where you duplicate logic everywhere.