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

androidclean codedesign-patternsdryjava

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.

Best Answer

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)?

Related Topic