1

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.

  • Which programming language? – Robert Harvey Mar 19 '18 at 20:28
  • @RobertHarvey Looks like java or c#. Is that really relevant for the overall design? – πάντα ῥεῖ Mar 19 '18 at 20:29
  • Yes, it is. Different languages have different idioms. – Robert Harvey Mar 19 '18 at 20:30
  • @RobertHarvey whoops, totally failed on that one lol. It's Java. – Chris Cirefice Mar 19 '18 at 20:35
  • 3
    Strictly speaking, DRY does not apply to mere method calls, but only to the method bodies. You would only consolidate method calls if you wanted to compose functionality, as in void showInterstitialAdConditionally() { if (!isSubscriptionActive() && isEventThresholdMet()) { showInterstitialAd(); } } Of course, you would need a new composition class that pulls together all of the necessary classes to make this happen. – Robert Harvey Mar 19 '18 at 20:49
7

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

  • Well, yes, but a better solution is one that doesn't require this kind of convoluted naming in the first place. – Robert Harvey Mar 19 '18 at 20:33
  • 2
    showInterstitialAdIfNecessary() – Renato Dinhani Mar 19 '18 at 23:05
  • @RobertHarvey No question there, but OP said that they didn't have the ability to make this event driven further down the page. That makes me think they don't have the luxury to refactor the entire thing either. This is a small change that makes it slightly more readable. – Deacon Mar 20 '18 at 12:28
0

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.

  • I would greatly prefer names like showInterstitialAdToGuests() and showInterstitialAdToAnyUser(). This isn't because one needs to promise to always work. It's because it's nice to be able to tell them apart. And of course user.showInterstitialAd() might call either of them. – candied_orange Mar 20 '18 at 18:18
0

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.

  • 1
    "You have no right to assume that any method named showInterstitialAd() actually does what it says it does" ... So function names are rough guidelines at-best, and entirely meaningless at worst? While it is true that it is impossible to give a complete description of what a function does, the essence of its purpose should be reflected in the function's name. – CharonX Mar 20 '18 at 12:17
  • Let us imagine an array with the function add(something). There is no need to tell in the function name that it may resize the array if full. But if it were to (silently!) discard the something instead, if the array is full, using your guidelines that would be ok too - which I think many programmers would find... counterintuitive to say the least. – CharonX Mar 20 '18 at 12:18
  • 1
    @CharonX It's only counterintuitive if you have the wrong mind set. Imagine you tell me to jump and I ignore you. Does it mean you got the name of the command wrong or does it mean you're not my drill sergeant? Abstraction and polymorphism only work when you stop thinking the name tells the whole story. This might not be how your array behaves but it's how the Null Object Pattern behaves. That doesn't make a name a rough guideline or meaningless. It means names aren't results. The ability to tell me to jump isn't a promise that I will jump. – candied_orange Mar 20 '18 at 14:10
  • If it is a command you might ignore, then what am I supposed to expect when I call a function? It might do what I ask of it, or it might silently(!) do nothing? Pardon me if I feel somewhat astonished if a add function does not add... You mention the Null Object Pattern - which is an entirely different animal, as a Null Object is used in lieu of an non-existing object (e.g. instead of a nullpointer). Calling add() on a null object does -always, predictably- nothing (instead of having to handle e.g. a nullpointer) – CharonX Mar 20 '18 at 14:57
  • @CharonX If you call add on an object (backed by array or not) that by design might not exist, or, by design doesn't accept additions on Tuesday you can expect it to deal with your add command appropriately. Since this isn't a failure you aren't owed an error. This design free's the client from needing to know about the implementation details. A name is not a post condition. – candied_orange Mar 20 '18 at 15:08
-3

!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.

  • 1
    Sorry, but I don't agree. You don't have to use events for everything; doing so can produce its own convolution. – Robert Harvey Mar 19 '18 at 20:36
  • 1
    [waves hand dismissively] – Robert Harvey Mar 19 '18 at 20:38
  • 1
    This is one case where an event listener would obviously make sense, but I don't have the luxury to use it at the moment, nor does this actually answer the question about the design of the code... – Chris Cirefice Mar 19 '18 at 20:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.