39

I would like to refactor some code that currently consists of a superclass and two subclasses.

These are my classes:

public class Animal {
    int a;
    int b;
    int c;
}

public class Dog extends Animal {
    int d;
    int e;
}

public class Cat extends Animal {
    int f; 
    int g;
}

This is my current code:

ArrayList<Animal> listAnimal = new ArrayList<>();

if (condition) {
    Dog dog = new Dog();
    dog.setA(..);
    dog.setB(..);
    dog.setC(..);
    dog.setD(..);
    dog.setE(..);   
    listAnimal.add(dog);

} else {
    Cat cat = new Cat();
    cat.setA(..);
    cat.setB(..);
    cat.setC(..);
    cat.setF(..);
    cat.setG(..);
    listAnimal.add(cat);
}

How can I refactor the code regarding the common attributes?

I would like something like that:

Animal animal = new Animal();
animal.setA(..);
animal.setB(..);
animal.setC(..);

if (condition) {
    Dog anim = (Dog) animal; //I know it doesn't work
    anim.setD(..);
    anim.setE(..);  
} else {
    Cat anim = (Cat) animal; //I know it doesn't work
    anim.setF(..);
    anim.setG(..);
}

listAnimal.add(anim);
slartidan
  • 20,403
  • 15
  • 83
  • 131
Jax Teller
  • 1,447
  • 2
  • 15
  • 24

10 Answers10

37

Your idea to have a variable of type Animal is good. But you also have to make sure to use the right constructor:

Animal animal; // define a variable for whatever animal we will create

if (condition) {
    Dog dog = new Dog(); // create a new Dog using the Dog constructor
    dog.setD(..);
    dog.setE(..);  
    animal = dog; // let both variables, animal and dog point to the new dog
} else {
    Cat cat = new Cat(); 
    cat.setF(..);
    cat.setG(..);
    animal = cat;
}

animal.setA(..); // modify either cat or dog using the animal methods
animal.setB(..);
animal.setC(..);

listAnimal.add(animal);

Hint: If an Animal is always either Cat or Dog consider making Animal abstract. Then the compiler will automatically complain whenever you try to do new Animal().

slartidan
  • 20,403
  • 15
  • 83
  • 131
15

The process of constructing either a cat or dog is complex since a lot of fields are involved. That is a good case for the builder pattern.

My idea is to write a builder for each type and organise relationships between them. It could be composition or inheritance.

  • AnimalBuilder constructs a general Animal object and manages the a, b, c fields
  • CatBuilder takes an AnimalBuilder(or extends it) and continues building a Cat object managing the f, g fields
  • DogBuilder takes an AnimalBuilder (or extends it) and continues building a Dog object managing the d, e fields

If you don't want to create builders, consider introducing a static factory method with a meaningful name for each subclass:

Animal animal = condition ? Dog.withDE(4, 5) : Cat.withFG(6, 7);
// populate animal's a, b, c
listAnimal.add(animal);

It would simplify construction and make it less verbose and more readable.

Andrew Tobilko
  • 48,120
  • 14
  • 91
  • 142
11

Answer

One way of doing it is to add the proper constructors to your classes. Look below:

public class Animal {
   int a, b, c; 

   public Animal(int a, int b, int c) {
      this.a = a;
      this.b = b;
      this.c = c;
   } 
}

public class Dog extends Animal {
   int d, e; 

   public Dog(int a, int b, int c, int d, int e) {
      super(a, b, c);
      this.d = d;
      this.e = e;
   } 
} 

public class Cat extends Animal {
   int f, g; 

   public Cat(int a, int b, int c, int f, int g) {
      super(a, b, c);
      this.f = f;
      this.g = g;
   } 
}

Now, to instantiate the objects, you can do as follows:

ArrayList<Animal> listAnimal = new ArrayList();

//sample values
int a = 10;
int b = 5;
int c = 20;

if(condition) {
   listAnimal.add(new Dog(a, b, c, 9, 11));
   //created and added a dog with e = 9 and f = 11
} 
else {
   listAnimal.add(new Cat(a, b, c, 2, 6));
   //created and added a cat with f = 2 and g = 6
} 

This is the method I would use in this case. It keeps the code cleaner and more readable by avoiding that tons of "set" methods. Note that super() is a call to the superclass' (Animal in this case) constructor.




Bonus

If you don't plan to create instances of the class Animal, you should declare it as being abstract. Abstract classes can't be instantiated, but can be subclassed and can contain abstract methods. Those methods are declared without a body, meaning that all the children classes must provide their own implementation of it. Here is an example:

public abstract class Animal {
   //...  

   //all animals must eat, but each animal has its own eating behaviour
   public abstract void eat();
} 

public class Dog {
   //... 

   @Override
   public void eat() {
     //describe the eating behaviour for dogs
   } 
}

Now you can call eat() for any and every animal! In the preceding example, with the list of animals, you would be able to do like below:

for(Animal animal: listAnimal) {
   animal.eat();
} 
Talendar
  • 1,841
  • 14
  • 23
4

Consider making your classes immutable (Effective Java 3rd Edition Item 17). If all parameters are required use a constructor or a static factory method (Effective Java 3rd Edition Item 1). If there are required and optional parameters use the builder pattern (Effective Java 3rd Edition Item 2).

Diego Marin Santos
  • 1,923
  • 2
  • 15
  • 29
4

I would consider a dynamic lookup/registration of capabilities/features: Flying/Swimming.

It is the question whether this fits your usage: instead of Flying & Swimming take Bird and Fish.

It depends whether the properties added are exclusive (Dog/Cat) or additive (Flying/Swimming/Mammal/Insect/EggLaying/...). The latter is more for a lookup using a map.

interface Fish { boolean inSaltyWater(); }
interface Bird { int wingSpan(); setWingSpan(int span); }

Animal animal = ...

Optional<Fish> fish = animal.as(Fish.class);
fish.ifPresent(f -> System.out.println(f.inSaltyWater()? "seafish" : "riverfish"));

Optional<Bird> bird = animal.as(Bird.class);
bird.ifPresent(b-> b.setWingSpan(6));

Animal need not implement any interface, but you can look up (lookup or maybe as) capabilities. This is extendible in the future, dynamic: can change at run-time.

Implementation as

private Map<Class<?>, ?> map = new HashMap<>();

public <T> Optional<T> as(Class<T> type) {
     return Optional.ofNullable(type.cast(map.get(type)));
}

<S> void register(Class<S> type, S instance) {
    map.put(type, instance);
}

The implementation does a safe dynamic cast, as register ensures the safe filling of (key, value) entries.

Animal flipper = new Animal();
flipper.register(new Fish() {
    @Override
    public boolean inSaltyWater() { return true; }
});
Joop Eggen
  • 107,315
  • 7
  • 83
  • 138
  • 1
    your `as` method can be simplified to `return Optional.ofNullable(map.get(type)).map(type.cast(instance))`, lovely names and examples otherwise! – Eugene Sep 26 '18 at 13:47
  • @Eugene thanks, at that moment I was unsure, but `type.cast(null)` is fine. I'll change to safe lines of code. – Joop Eggen Sep 26 '18 at 14:23
4

As an alternative, you can make the "Animal" parts of your dog and cat a separate entity that is available via the "Animalian" interface. By doing this, you're creating the common state first and then providing it to the species specific constructor at the point it is needed.

public class Animal {
    int a;
    int b;
    int c;
}

public interface Animalian {
    Animal getAnimal();
}

public class Dog implements Animalian {
    int d;
    int e;
    Animal animal;
    public Dog(Animal animal, int d, int e) {
        this.animal = animal;
        this.d = d;
        this.e = e;
    }
    public Animal getAnimal() {return animal};
}

public class Cat implements Animalian {
    int f;
    int g;
    Animal animal;
    public Cat(Animal animal, int f, int g) {
        this.animal = animal;
        this.f = f;
        this.g = g;
    }
    public Animal getAnimal() {return animal};
}

Now to create animals:

Animal animal = new Animal();
animal.setA(..);
animal.setB(..);
animal.setC(..);

if (condition) {
    listAnimalian.add(new Dog(animal, d, e));
} else {
    listAnimalian.add(new Cat(animal, f, g));
}

The reason for doing this is to "favor composition over inheritance". I want to express that this is merely a different way of solving the problem posed. It doesn't mean that composition should be favored over inheritance at all times. It's up to the engineer to determine the correct solution for the context in which the problem arises.

There is a lot of reading on this topic.

Matsemann
  • 21,083
  • 19
  • 56
  • 89
justin.hughey
  • 1,246
  • 15
  • 16
4

Here is a solution, quite close of slartidan's one but using setter with builder's style, avoiding the dog and cat variables

public class Dog extends Animal
{
    // stuff

    Dog setD(...)
    {
        //...
        return this;
    }

    Dog setE(...)
    {
        //...
        return this;
    }
}

public class Cat extends Animal
{
    // stuff

    Cat setF(...)
    {
        //...
        return this;
    }

    Cat setG(...)
    {
        //...
        return this;
    }
}

Animal animal = condition ?
    new Dog().setD(..).setE(..) :
    new Cat().setF(..).setG(..);

animal.setA(..);
animal.setB(..);
animal.setC(..);

listAnimal.add(animal);
ToYonos
  • 16,469
  • 2
  • 54
  • 70
  • It's a very strange solution: setters are supposed to set the value and return nothing. Also, your subclasses will have different setter types: standard ones (for a, b, c) and those that return `this`. – Andrew Tobilko Oct 06 '18 at 20:07
  • A, b, and c setters can also return this – ToYonos Oct 06 '18 at 21:07
4

Here is what I would like to propose:

import java.util.ArrayList;
import java.util.List;

class Animal {
    int a;
    int b;
    int c;

    public Animal setA(int a) {
        this.a = a;
        return this;
    }

    public Animal setB(int b) {
        this.b = b;
        return this;
    }

    public Animal setC(int c) {
        this.c = c;
        return this;
    }
}

class Dog extends Animal {
    int d;
    int e;

    public Dog setD(int d) {
        this.d = d;
        return this;
    }

    public Dog setE(int e) {
        this.e = e;
        return this;
    }
}

class Cat extends Animal {
    int f;
    int g;

    public Cat setF(int f) {
        this.f = f;
        return this;
    }

    public Cat setG(int g) {
        this.g = g;
        return this;
    }
}

class Scratch {
    public static void main(String[] args) {
        List<Animal> listAnimal = new ArrayList();
        boolean condition = true;
        Animal animal;
        if (condition) {
            animal = new Dog()
                    .setD(4)
                    .setE(5);

        } else {
            animal = new Cat()
                    .setF(14)
                    .setG(15);
        }
        animal.setA(1)
                .setB(2)
                .setC(3);
        listAnimal.add(animal);

        System.out.println(listAnimal);
    }
}

Some noteworthy points:

  1. Use of List interface in declaration List<Animal> listAnimal
  2. Use of interface animal while object creation Animal animal;
  3. abstract class Animal
  4. Setters returning this to make the code cleaner. Or you would have to use code like animal.setD(4); animal.setE(5);

This way we can utilize the interface Animal and set the common attributes once. Hope this helps.

mayurmadnani
  • 189
  • 4
1

Refactor your code to:

ArrayList<Animal> listAnimal = new ArrayList();

//Other code...

if (animalIsDog) {
    addDogTo(listAnimal, commonAttribute, dogSpecificAttribute); 
} else {
    addCatTo(listAnimal, commonAttribute, catSpecificAttribute);
}

Benefits with the new code:

  1. Hidden complexity: You have hidden the complexity away and you will now have to look at a way smaller code, written almost in plain english, when revisiting your code later.

But now, the methods addDogTo and addCatTo would have to be written. This is how they would look like:

private void addDogTo(ArrayList<Animal> listAnimal,
    AnimalAttribute generalAttribute,
    DogAttribute specificAttribute) {
    var dog = createDog(commonAttribute, specificAttribute);
    listAnimal.add(dog);
}

private void addCatTo(ArrayList<Animal> listAnimal,
    AnimalAttribute generalAttribute,
    CatAttribute specificAttribute) {
    var cat = createCat(commonAttribute, specificAttribute);
    listAnimal.add(cat);
}

Benefits:

  1. Hidden complexity;
  2. Both methods are private: This means that they can only be called from within the class. So you can safely do away with checking the input for null etc. because the caller (which is within the class) must have taken care to not pass spurious data to its own members.

This means that now we need to have createDog and createCat methods in place. This is how I would write those methods:

private Dog createDog(AnimalAttribute generalAttribute,
    DogAttribute specificAttribute) {
    var dog = new Dog(generalAttribute, specificAttribute);
    return dog;
}

private Cat createCat(AnimalAttribute generalAttribute,
    CatAttribute specificAttribute) {
    var cat = new Cat(generalAttribute, specificAttribute);
    return cat;
}

Benefits:

  1. Hidden complexity;
  2. Both methods are private.

Now, for the above-written code, you will have to write constructors for Cat and Dog that take in the common attributes and the specific attributes for object construction. That can look like:

public Dog(AnimalAttribute generalAttribute,
    DogAttribute specificAttribute)
        : base (generalAttribute) {
    this.d = specificAttribute.getD();
    this.e = specificAttribute.getE();
}

and,

public Cat(AnimalAttribute generalAttribute,
    CatAttribute specificAttribute)
        : base (generalAttribute) {
    this.f = specificAttribute.getF();
    this.g = specificAttribute.getG();
}

Benefits:

  1. Code is DRY: Both constructors call the superclass method with generalAttributes and that takes care of common attributes of both sub-class objects;
  2. The whole object is preserved: Instead of calling a constructor and passing it 20 thousand parameters, you are just passing it 2, viz. general animal attribute object and specific animal attribute object. These two parameters hold the rest of the attributes within and they are unboxed inside the constructor when needed.

Finally, your Animal's constructor will look like:

public Animal(AnimalAttribute attribute) {
    this.a = attribute.getA();
    this.b = attribute.getB();
    this.c = attribute.getC();
}

Benefits:

  1. The whole object is preserved;

For the sake of completion:

  • AnimalAttribute/DogAttribute/CatAttribute classes only have some fields and the getters and setters for those fields;
  • These fields are the data needed to construct the Animal/Dog/Cat object.
displayName
  • 13,888
  • 8
  • 60
  • 75
0

Many great suggestions here. I would use my personal favorite builder pattern (but with added inheritance flavor):

public class Animal {

    int a;
    int b;
    int c;

    public Animal() {
    }

    private <T> Animal(Builder<T> builder) {
        this.a = builder.a;
        this.b = builder.b;
        this.c = builder.c;
    }

    public static class Builder<T> {
        Class<T> builderClass;
        int a;
        int b;
        int c;

        public Builder(Class<T> builderClass) {
            this.builderClass = builderClass;
        }

        public T a(int a) {
            this.a = a;
            return builderClass.cast(this);
        }

        public T b(int b) {
            this.b = b;
            return builderClass.cast(this);
        }

        public T c(int c) {
            this.c = c;
            return builderClass.cast(this);
        }

        public Animal build() {
            return new Animal(this);
        }
    }
    // getters and setters goes here 

}

public class Dog extends Animal {

    int d;
    int e;

    private Dog(DogBuilder builder) {
        this.d = builder.d;
        this.e = builder.e;
    }

    public static class DogBuilder extends Builder<DogBuilder> {
        int d;
        int e;

        public DogBuilder() {
            super(DogBuilder.class);
        }

        public DogBuilder d(int d) {
            this.d = d;
            return this;
        }

        public DogBuilder e(int e) {
            this.e = e;
            return this;
        }

        public Dog build() {
            return new Dog(this);
        }
    }
    // getters and setters goes here 
}

public class Cat extends Animal {

    int f;
    int g;

    private Cat(CatBuilder builder) {
        this.f = builder.f;
        this.g = builder.g;
    }

    public static class CatBuilder extends Builder<CatBuilder> {
        int f;
        int g;

        public CatBuilder() {
            super(CatBuilder.class);
        }

        public CatBuilder f(int f) {
            this.f = f;
            return this;
        }

        public CatBuilder g(int g) {
            this.g = g;
            return this;
        }

        public Cat build() {
            return new Cat(this);
        }
    }
    // getters and setters goes here 
}

public class TestDrive {

    public static void main(String[] args) {

        Boolean condition = true;
        ArrayList<Animal> listAnimal = new ArrayList<>();

        if (condition) {
            Dog dogA = new Dog.DogBuilder().a(1).b(2).c(3).d(4).e(5).build();
            Dog dogB = new Dog.DogBuilder().d(4).build();
            listAnimal.add(dogA);
            listAnimal.add(dogB);

        } else {
            Cat catA = new Cat.CatBuilder().b(2).f(6).g(7).build();
            Cat catB = new Cat.CatBuilder().g(7).build();
            listAnimal.add(catA);
            listAnimal.add(catB);
        }
        Dog doggo = (Dog) listAnimal.get(0);
        System.out.println(doggo.d); 
    }
}

Note: The Animal.Builder constructor takes Class builderClass as generic argument. Cast the current instance of object to this class when it return.

nabster
  • 1,561
  • 2
  • 20
  • 32