CODEWITHSUNDEEP
javaoopdesign-patterns
Rami
Rami

Reputation: 7290

Ignoring method parameters

I have a static private java method that has been used in many places in the containing class.

It is something like:

private static ColoredItem createItem(Object obj, Color background, Color foreground) {
        ColoredItem item = new ColoredItem();
        //Some logic goes here to paint the item
        //Some other logic to apply the background and foreground colors
        return item;
    }

My question is, from a good design perspective, is it appropriate to ignore the colors passed as parameters to the method in case the first parameter implements a specific interface?

So the method will be modified to be something like:

private static ColoredItem createItem(Object obj, Color background, Color foreground) {
        ColoredItem item = new ColoredItem();
        //Some logic goes here to paint the item
        //Some other logic to apply the background and foreground colors
        if(obj instanceof SOME_INTERFACE) {
              item.setBackgroundColor(some other color);
              item.setForegroundColor(some other color);
        }
        return item;
    }

Upvotes: 2

Views: 4473

Answers (7)

ss_everywhere
ss_everywhere

Reputation: 479

I would recommend moving the if condition to a Factory class. Here is a pseudo implementation.

ColoredItemFactory will return an implementation of ColoredItem interface that has a create method.

The ColoredItem interface:

    interface ColoredItem {
    }
    class AColoredItem implements ColoredItem{
      AColoredItem(Color... colors) {
//      Do something specific to A here
      }
    }
    class BColoredItem implements ColoredItem{
      BColoredItem(Color... colors) {
//      Do something specific to B here
      }
    }

The ColoredItemFactory:

class ColoredItemFactory {
  public static ColoredItem getColoredItem(ColoredItemType type, Color... colors) {
    switch(type) {
     case A:
       return new AColoredItem(Color... colors);
       break;
     case B:
       return new BColoredItem(Color... colors);
       break;
    }
  }
}

enum ColoredItemType {
 A,B;
}

ColoredItemFactory will return the correct implementation of the ColoredItem. The ColoredItemType enum helps the ColoredItemFactory figure out the correct implementation class. Hope this helps!

Upvotes: 0

Rafa
Rafa

Reputation: 2368

Instead of overloading the constructor (what you can do, for sure) I would recommend you to use the Builder Pattern, it allows you to decide exactly how to build your object depending on the parameters you have:

https://en.wikipedia.org/wiki/Builder_pattern

The builder pattern is an object creation software design pattern. Unlike the abstract factory pattern and the factory method pattern whose intention is to enable polymorphism, the intention of the builder pattern is to find a solution to the telescoping constructor anti-pattern. The telescoping constructor anti-pattern occurs when the increase of object constructor parameter combination leads to an exponential list of constructors. Instead of using numerous constructors, the builder pattern uses another object, a builder, that receives each initialization parameter step by step and then returns the resulting constructed object at once.

I found this pattern in a lot of java code when using some classes from Google and I'm a big fan since then.

Upvotes: 0

Terrence
Terrence

Reputation: 116

I guess the parameter 'background' and 'foreground' would be set to ColoredItem in the 'some other logic' no matter what the obj is. So I would like do it like this:

public interface CustomInterface{
    Color getBackgroundColor(Color orginBackground);
    Color getForegroundColor(Color orginForeground);
}

private static ColoredItem createItem(CustomInterface obj, Color background, Color foreground) {
    ColoredItem item = new ColoredItem();
    //Some logic goes here to paint the item
    //Some other logic to apply the background and foreground colors
    item.setBackgroundColor(obj.getBackgroundColor(background));
    item.setForegroundColor(obj.getForegroundColor(foreground));
    return item;
}

Upvotes: 1

user784540
user784540

Reputation:

From the design perspective it is better to avoid use of instanceof operator and use a declared interface type as the obj parameter type.

Declare an interface, let say MyCustomInterface

public interface MyCustomInterface {

   public void setBackgroundColor(int color);

   public void setForegroundColor(int color);

}

and declare 

private static ColoredItem createItem(MyCustomInterface obj) {
        ColoredItem item = new ColoredItem();

        int bgColor = .... 
        int color = ....      

        obj.setBackgroundColor(bgColor);
        obj.setForegroundColor(color);
        return item;
}

or even better way:

Add MyCustomInterface parameter to the ColoredItem constructor. In the class ColoredItem declare default background and foreground colors. And use them if these colors are not specified in the parameter.

Then use:

private static ColoredItem createItem(MyCustomInterface obj) {
        return new ColoredItem(obj);
}

Upvotes: 2

Caius Brindescu
Caius Brindescu

Reputation: 607

No, it is not. A good design option is to use overloading:

private static ColoredItem createItem(Interface1 obj, Color background, Color foreground){

// do something here that require the two parameters
}


private static ColoredItem createItem(Interface2 obj) {
/// do something for the object that do not require the two other parameters
}

This way, you avoid checking the type of your first parameter. The runtime will do this for you. Also, the code will be cleaner, since now the users of this method will know exactly what to pass to it.

I hope this helps, -Caius

Upvotes: 3

Jack
Jack

Reputation: 133577

in general instanceof should be avoided when there are other solutions to your problem, this because instanceof is a runtime check but you have many ways to select appropriate behavior at compile time.

If you are allowed to know at compile time the type of the variable you are working on then you can just let method overloading do the dirty work:

ColoredItem createItem(SomeInterface obj, Color background, Color foreground) {
  ColoredItem item = createItem(obj);
  item.setBackground(...);
}

ColoredItem createItem(GenericItem obj) {
  ...
}

Upvotes: 2

Baldy
Baldy

Reputation: 2002

I would use a separate method, document it appropriately and then call the existing method with the new parameters. It should work, depending of course on your actual object hierarchy.

private static ColoredItem createItem(SOME_INTERFACE obj) {
    return createItem(obj, newColor1, newColor2);
}

Upvotes: 2

Related Questions