13

Basically I need to execute different actions given a certain condition. The existing code is written this way

Base interface

// DoSomething.java
interface DoSomething {

   void letDoIt(String info);
}

Implementation of the first worker class

class DoItThisWay implements DoSomething {
  ...
}

Implementation of the second worker class

class DoItThatWay implements DoSomething {
   ...
}

The main class

   class Main {
     public doingIt(String info) {
        DoSomething worker;
        if (info == 'this') {
          worker = new DoItThisWay();
        } else {
          worker = new DoItThatWay();
        }
        worker.letDoIt(info)
     }

This code works ok and is easy to understand.

Now, due to a new requirement, I need to pass a new piece of information that is only make sense to DoItThisWay.

My question is: does the following coding style good to handle this requirement.

Use new class variable and method

// Use new class variable and method

class DoItThisWay implements DoSomething {
  private int quality;
  DoSomething() {
    quality = 0;
  }

  public void setQuality(int quality) {
    this.quality = quality;
  };

 public void letDoIt(String info) {
   if (quality > 50) { // make use of the new information
     ...
   } else {
     ...
   }
 } ;

}

If I do it this way, I need to make the corresponding change to the caller:

   class Main {
     public doingIt(String info) {
        DoSomething worker;
        if (info == 'this') {
          int quality = obtainQualityInfo();
          DoItThisWay tmp = new DoItThisWay();
          tmp.setQuality(quality)
          worker = tmp;

        } else {
          worker = new DoItThatWay();
        }
        worker.letDoIt(info)
     }

Is it a good coding style? Or can I just cast it

   class Main {
     public doingIt(String info) {
        DoSomething worker;
        if (info == 'this') {
          int quality = obtainQualityInfo();
          worker = new DoItThisWay();
          ((DoItThisWay) worker).setQuality(quality)
        } else {
          worker = new DoItThatWay();
        }
        worker.letDoIt(info)
     }
  • 19
    Why not just pass quality into the constructor for DoItThisWay? – David Arno Jun 7 at 8:24
  • I probably need to revise my question... because for performance reason the construction of DoItThisWay and DoItThatWay are done once in the constructor of Main. Main is a long living class and doingIt is called many times over and over. – Anthony Kong Jun 7 at 8:28
  • Yes, updating your question would be a good idea in that case. – David Arno Jun 7 at 8:58
  • Do you mean to say that the setQuality method will be called multiple times during the lifetime of the DoItThisWay object? – jpmc26 Jun 7 at 17:11
  • 1
    There is no relevant difference between the two versions you presented. From a logical standpoint, they do the same thing. – Sebastian Redl Jun 8 at 8:21
6

I'm assuming that quality needs to be set alongside each letDoIt() call on a DoItThisWay().

The issue I see arising here is this: You are introducing temporal coupling (i.e. what happends if you forget to call setQuality() before calling letDoIt() on a DoItThisWay?). And the implementations for DoItThisWay and DoItThatWay are diverging (one needs to have setQuality() called, the other not).

While this might not cause issues right now, it may come back to haunt you eventually. It might be worthwhile to take another look at letDoIt() and consider if the quality information might need to be part of the info you pass through it; but this depends of the details.

17

Is it a good coding style?

From my point of view neither of your versions is. First having to call setQuality before letDoIt can be called is a temporal coupling. You're stuck viewing DoItThisWay as an derivative of DoSomething, but it isn't (at least not functionally), it's rather something like a

interface DoSomethingWithQuality {
   void letDoIt(String info, int quality);
}

This would make Main rather something like

class Main {
    // omitting the creation
    private DoSomething doSomething;
    private DoSomethingWithQuality doSomethingWithQuality;

    public doingIt(String info) {
        DoSomething worker;
        if (info == 'this') {
            int quality = obtainQualityInfo();
            doSomethingWithQuality.letDoIt(info, quality);
        } else {
            doSomething.letDoIt(info);
        }
    }
}

You could on the other hand pass the parameters to the classes directly (assuming this is possible) and delegate the decision which one to use to a factory (this would make the instances interchangeable again and both can be derived from DoSomething). This would make Main look like this

class Main {
    private DoSomethingFactory doSomethingFactory;

     public doingIt(String info) {
         int quality = obtainQualityInfo();
         DoSomething doSomethingWorker = doSomethingFactory.Create(info, quality);
         doSomethingWorker.letDoIt();
     }
}

I am aware that you wrote that

because for performance reason the construction of DoItThisWay and DoItThatWay are done once in the constructor of Main

But you could cache the parts that are costly to create in the factory and pass them to the constructor, too.

  • Argh, are you spying on me while I type answers? We make such similar points (but yours is much more comprehensive and eloquent) ;) – CharonX Jun 7 at 9:19
  • 1
    @DocBrown Yeah, it's debatable, but since the DoItThisWay requires the quality to be set before calling letDoIt it behaves differently. I'd expect a DoSomething to work the same way (not setting the quality before) for all derivatives. Does this make sense? Of course this is kind of blurry, since if we called the setQuality from a factory method the client would be agnostic about whether the quality has to be set or not. – Paul Kertscher Jun 7 at 10:04
  • 2
    @DocBrown I'd assume the contract for letDoIt() is (lacking any additional info) "I can correctly execute (do whatever I do) if you provide me with String info". Requiring that setQuality() is called beforehand strengthens the precondition of calling letDoIt(), and thus would be a violation of LSP. – CharonX Jun 7 at 11:37
  • 2
    @CharonX: if, for example, there would be a contract implying that "letDoIt" shall not throw any exceptions, and forgetting to call "setQuality" would make letDoIt then throw an exception, then I would agree, such an implementation would violate the LSP. But those look like very artificial assumptions to me. – Doc Brown Jun 7 at 12:43
  • 1
    This is a good answer. The only thing I would add is some commentary on how disastrously bad temporal coupling is for a code base. It's a hidden contract between seemingly decoupled components. With normal coupling, at least it's explicit and easily identifiable. Doing this is essentially digging a pit and covering it with leaves. – JimmyJames Jun 13 at 13:37
11

Let's assume quality cannot be passed into the constructor, and the call to setQuality is required.

Currently, a code snippet like

    int quality = obtainQualityInfo();
    worker = new DoItThisWay();
    ((DoItThisWay) worker).setQuality(quality);

is way too small to invest too much thoughts into it. IMHO it looks a little bit ugly, but not really hard to understand.

Problems arise when such a code snippet grows and due to refactoring you end up with something like

    int quality = obtainQualityInfo();
    worker = CreateAWorkerForThisInfo();
    ((DoItThisWay) worker).setQuality(quality);

Now, you don't see immediately if that code is still correct, and the compiler won't tell you. So introducing a temporary variable of the correct type, and avoiding the cast, is a little bit more type-safe, without any real extra effort.

However, I would actually give tmp a better name:

      int quality = obtainQualityInfo();
      DoItThisWay workerThisWay = new DoItThisWay();
      workerThisWay.setQuality(quality)
      worker = workerThisWay;

Such naming helps to make wrong code look wrong.

  • tmp might be even better named something like "workerThatUsesQuality" – user949300 Jun 7 at 9:16
  • 1
    @user949300: IMHO that's not a good idea: assume DoItThisWay gets more specific parameters - by what you suggesting, the name needs to be changed every time. It is better to use names vor variables which make the object type clear, not which methods names are available. – Doc Brown Jun 7 at 9:32
  • To clarify, it would be the name of the variable, not the name of the class. I agree that putting all the capabilities in the class name is a poor idea. So I'm suggesting that workerThisWay become something like usingQuality. In that small code snippet it is safer to be more specific. (And, if there is a better phrase within their domain than "usingQuality" use it. – user949300 Jun 7 at 14:15
2

A common interface where the initialization varies based on runtime data usually lends itself to the factory pattern.

DoThingsFactory factory = new DoThingsFactory(thingThatProvidesQuality);
DoSomething doSomething = factory.getDoer(info);

doSomething.letDoIt();

Then DoThingsFactory can worry about getting and setting the quality information inside the getDoer(info) method prior to returning the concrete DoThingsWithQuality object cast to the DoSomething interface.

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.