4
\$\begingroup\$

Yesterday I posted my first solution to an interview problem here. I am now aware that I have many weak spots in Java and need to do extensive review before tackling any more interviews. Having said that, piecing together what people on here mentioned, I have come to what I hope is a good solution, and am asking to have it reviewed again.

Here is the problem:

You've gone back in time to 500BC Athens and Socrates wants you to build him an app to help classify animals.

  1. Build the classes Animal, Cat, and Bug.
  2. Define the properties "color" and "leg_number" on the relevant and necessary classes. Have them be initialized within a constructor.
  3. Add the functionality that would allow us to call a method "move" with the Cat and Bug classes. Have the method return a string "I'm moving with <number of legs> legs!", with the "<number of legs>" being leg_number as set on the class.
  4. Add a new class called Bird. Add the property "wing_number". Add the functionality that would allow us to call a method "move" with the Bird class. Have the method return the string "I'm moving with <number of legs> legs!" if wing_number doesn't have an applicable value. If wing_number does have an applicable value, return the string "I'm flying".

Here was my final solution that I submitted:

public class SocratesApp {

    public static void main(String[] args) {
        Cat myCat = new Cat();
        System.out.println(myCat.move());
    
        Bug myBug = new Bug();
        System.out.println(myBug.move());
    
        Bird myBird1 = new Bird(2);
        System.out.println(myBird1.move());
    
        Bird myBird2 = new Bird(0);
        System.out.println(myBird2.move());
    }
}

class Animal {
    protected String color;
    protected int leg_number;

    public Animal() {

    }

    public String move() {
        return "I'm walking with " + leg_number + " legs!";
    }

}

class Cat extends Animal {
    public Cat() {
        color = "orange";
        leg_number = 4;
    }
}

class Bug extends Animal {
    public Bug() {
        color = "green";
        leg_number = 6;
    }
}

class Bird extends Animal {
    private int wing_number;

    public Bird(int wing_number) {
        color = "yellow";
        leg_number = 2;
        this.wing_number = wing_number;
    }

    //  @Override
    public String move() {
        if (this.wing_number > 0) {
            return "I'm flying";
        } else {
            return "I'm walking with " + leg_number + " legs!";
        }
    }
} 

A couple of observations of my one:

First, since this consists of a superclass and subclasses, I thought I was required to reference the superclass constructor in my subclass constructors with super(). However, I found out the code worked without it. I don't know if I was wrong about this requirement or if it was an old requirement that changed with the evolution of Java.

Second, I thought that the @Override decoration was required for the move() method in the Bird class due to the move() method in the Animal superclass. However, the code works without it. I'm thinking now that because the move() method in Bird has a parameter and the one in Animal does not, the Bird one does not constitute a true override of the Animal one.

Third, I was sure that to instantiate my different objects, the instantiation would somehow have to include the Animal superclass. When reviewing code examples on the web, I would see superclasses referenced like Animal myCat = new Cat() or, I think, Cat myCat = new Animal(), maybe one other way of referencing Animal that I don't remember. It turns out that Cat myCat = new Cat() was the only one that worked correctly. I'm not sure what the difference is between the code examples I saw on the Internet and my code, but I went with what worked.

Any comments on my observations as well as my code are welcome.

\$\endgroup\$
6
  • \$\begingroup\$ I am so glad I never had to do a code interview. I have been writing code since the '70s (and still am). Solving problems is a worthwhile skill to test, checking your skills as a compiler is a waste of time. \$\endgroup\$
    – copper.hat
    Oct 27 at 5:35
  • \$\begingroup\$ OT, but Socrates won't be born for another 30 years in 500 BC \$\endgroup\$
    – mcalex
    Oct 27 at 8:06
  • \$\begingroup\$ The fourth requirement is weird to me. As far as I know, all birds have two wings (unless they have a birth defect, mutation, or are mutilated). Not all birds fly, though. \$\endgroup\$ Oct 27 at 8:26
  • \$\begingroup\$ I'd find somewhere else to apply to. Animal inheritance is a very poor fit for Java inheritance and is a terrible programming challenge to give to candidates. Only birds without wings walk? Penguins and ostriches have wings, they just don't use them to fly! \$\endgroup\$ Oct 27 at 8:55
  • \$\begingroup\$ "However, the code works without it. I'm thinking now that because the move() method in Bird has a parameter and the one in Animal does not" Was this edited out? Because both methods have no parameters. \$\endgroup\$
    – Flater
    Oct 27 at 14:46
6
\$\begingroup\$

This is better, but you are still missing a lot of basic ideas. I'm going to continue to approach the code review by looking at concepts and suggesting what you need to consider and what you should probably review in your learning, rather than by doing line-wise analysis and suggesting alternatives.

  1. Will you ever want to instantiate a generic Animal object? I don't think so. So what should you do about the Animal class?
  2. Are all cats orange, all bugs green and all birds yellow?
  3. Do all cats have 4 legs? I've encountered 3-legged cats before.
  4. I assume bugs includes ants, spiders and millipedes. They don't all have 6 legs.
  5. The move() method in Bird doesn't have a parameter. You should review your understanding of Java terminology, or you won't look very clever in an interview.
  6. Wingless birds walk like the other Animals. So what does that suggest about the "zero wings" path in the move() method?

Regarding calling the constructor of the superclass - given that the Animal constructor does nothing, what value would calling it serve? An empty constructor may as well be omitted, if it is the only constructor for a class.

The Animal constructor could serve more purpose if you made the color and number of legs fields private, and set them in the Animal constructor. (Incidentally, calling it legNumber would be more in keeping with Java conventions, while calling it numberOfLegs might be more expressive. The same applies to number of wings. However, if that's the way the exercise was worded, I suppose you should follow the direction given - though it makes me less confident about the Java expertise of the assignment setter.)

The @Override annotation is documentation, and if omitted may lead to compiler warnings. Leaving it out won't cause your code to fail.

Your third question shows a lack of understanding of polymorphism in Java. All Cats are Animals in your example (all instances of subclasses are instances of their superclass) but the converse isn't true (instances of a superclass aren't instances of any of its subclasses). You can say

Animal myCat = new Cat();

but you can't say

Cat myCat = new Animal();

I think you need to spend more time studying the language, as these are fairly basic concepts.

\$\endgroup\$
2
  • \$\begingroup\$ Questions #1 and #5 are valid ones to pose to OP. The rest of them I would argue are overly cryptic as phrased. For example, from what I can tell, OP likely lacks the context necessary to understand what you are trying to get at by asking whether all cats are orange and have four legs or what to do when birds walk like other animals. \$\endgroup\$
    – Abion47
    Oct 27 at 1:15
  • \$\begingroup\$ @Albion47 my response was based on my having read and responded to OP's previous post. He's already used constuctors with parameters for color and legs, so points 2, 3 and 4 shouldn't be too difficult for him to understand. He's also shown he's aware of calling superclass methods (at least the superclass constructor) so point 6 shouldn't be too much trouble either. My aim is to get him thinking, and learning, but not to think that CodeReview is the place to learn Java from scratch. \$\endgroup\$ Oct 27 at 7:22
1
\$\begingroup\$

First, responding to your observations:

All subclass constructors implicitly start with super(); if they do not have an explicit superclass constructor call. However, if the superclass does not have a (non-private) constructor with no parameters, then the implicit super() would be calling a constructor that does not exist. In this situation, and only in this situation, you must always explicitly invoke a superclass constructor, because the compiler cannot deduce what the arguments of the call should be.

The @Override annotation (that's the official term in Java, not "decoration") is partly documentation and partly an optional protection against certain kinds of mistakes. It was not present in the original version of the language, and for backwards compatibility it was not made required when it was added. The optional protection part is that, if the override's method signature does not match any superclass method, then @Override will cause a compile error to draw attention to the mismatch.

Animal myCat = new Cat() should work, because all cats are animals, as indicated by the subclass relationship. Cat myCat = new Animal() is a compile error, because the subclass relationship is in the wrong direction for that. Cat myCat = new Cat() works because all cats, obviously, are cats. In any case, the Animal superclass is involved indirectly by the fact that the Cat() constructor, whether explicitly written to do so or not, calls the Animal() constructor.

Now, on to commenting about your code:

In general, protected should only be used when there is a reason for subclasses to have to do something directly with the protected thing. One common reason is when a method is declared for the specific purpose of allowing subclasses to add their own behavior to a certain point. For example, consider how to alter this to make cleanup() do its job properly:

class Superclass {
    // various other stuff

    public void cleanup() {
        // clean up superclass stuff that needs it
        // should also cleanup subclass stuff, but doesn't know how
    }
}

class Subclass extends Superclass {
    // various subclass stuff, some of which needs cleaning up
}

You could modify Subclass like this:

class Subclass extends Superclass {
    // various subclass stuff, some of which needs cleaning up

    @Override
    public void cleanup() {
        super.cleanup();
        // clean up the subclass stuff
    }
}

That approach works, but there's a problem with it: it's really easy to just forget to put the super.cleanup(); call in, and omitting that prevents the superclass cleanup from happening. Or, if this is in library code that might be extended by other people, the users of your library can modify how cleaning up the superclass stuff is done for objects of their subclass, and may break important parts of your library's functionality by doing so, potentially even in some intentionally malicious way.

This solution avoids that problem:

class Superclass {
    // various other stuff

    public final void cleanup() {
        // clean up superclass stuff that needs it
        subCleanup();
    }

    protected void subCleanup() {}
}

class Subclass extends Superclass {
    // various subclass stuff, some of which needs cleaning up

    @Override
    protected void subCleanup() {
        // clean up subclass stuff that needs it
    }
}

With this approach, subclasses cannot prevent or alter how the superclass gets cleaned up because cleanup() is final and thus cannot be overridden. Despite that, subclasses can get their own stuff cleaned up whenever cleanup() is called by overriding subCleanup() instead because cleanup() calls that.

In conclusion of this point, color and leg_number should be private, not protected. Note that this will require the Animal constructor to initialize them. It can handle the different values by adding parameters to the constructor, and then having subclasses call it like this: super("orange", 4);

If you simply make that change and try to compile, you will get a compile error for the Bird override of move() attempting to access a superclass private field. This does not mean that leg_number needs to be protected after all, however. Notice that, in the case where leg_number gets used, the returned value is identical to the superclass version of the method. You can simultaneously eliminate this code duplication (which would be worthwhile as its own point anyway) and resolve the compile error by replacing that line with return super.move();. This will invoke the Animal superclass version of move(), which does have access to private fields of Animal.

\$\endgroup\$
1
\$\begingroup\$

Use a strategy to implement the diverging behaviour in the animals: https://refactoring.guru/design-patterns/strategy

I added a Java scratch file that you can peruse for further insights. If someone other finds the code teaches wrongly, please let me know so I will myself be reviewed.

class Scratch {
    public static void main(String[] args) {
        // abstract Animal
        Animal abstractCat = new Cat("abstract and a bit of red");
        abstractCat.move();
        // concrete Animal
        Cat concreteCat = new Cat("concrete and tigerlike");
        concreteCat.move();
        // change bug behaviour (move()) whilst program is running by assigning new AnimalBehaviour interface.
        new Bug("iridium").move().setAnimalBehaviour(new AnimalMoveBehaviour()).move();
        //as AnimalBehaviour is an interface resp. a SAM, we can define its functionality via a lambda.
        new Bug(5, 3, "chaotic", animal -> System.out.println(animal.color +" colored animal moves with " +animal.legs + " legs and flies with " +animal.wings + "wings")).move();
    }
}
// define what functionality / behaviour an object should fulfil.
interface AnimalBehaviour {
    void move(Animal animal);
}

// one could think of a ProtoAnimal, which then is divided into Flying and Walking, or both, or even Swimming.
abstract class Animal {
    int legs, wings;
    String color;
    // inject behaviour (which is basically a contract) into class
    AnimalBehaviour animalBehaviour;
    public Animal(int legs, int wings, String color, AnimalBehaviour animalBehaviour) {
        this.legs = legs;
        this.color = color;
        this.wings = wings;
        this.animalBehaviour = animalBehaviour;
    }

    // setAnimalBehaviour enables changing the behaviour during runtime.
    // see main method for example.

    public Animal setAnimalBehaviour(AnimalBehaviour animalBehaviour) {
        this.animalBehaviour = animalBehaviour;
        // fluent interface to return instance so method-chaining becomes possible.
        return this;
    }

    // delegate Animal's move method (AnimalBehaviour) to interface.
    public Animal move(){
        this.animalBehaviour.move(this);

        // fluent interface to return instance so method-chaining becomes possible.
        return this;
    }
}

class Cat extends Animal {
    // standard cat
    public Cat(String color) {
        super(4, 0, color,  new AnimalMoveBehaviour());
    }

    // cat whose parameters can be passed during instantiation
    public Cat(int legs, int wings, String color, AnimalBehaviour ab) {
        super(legs, wings, color,  ab);
    }

}

class Bug extends Animal {
    // standard bug
    public Bug(String color) {
        super(6, 4, color, new AnimalFlyBehaviour());
    }

    // bug whose parameters can be passed during instantiation
    public Bug(int legs, int wings, String color, AnimalBehaviour ab) {
        super(legs, wings, color, ab);
    }
}

class AnimalMoveBehaviour implements AnimalBehaviour {

    @Override
    public void move(Animal animal) {
        System.out.println(animal.color + " colored animal is moving with " + animal.legs + " legs");
    }
}

class AnimalFlyBehaviour implements AnimalBehaviour {

    @Override
    public void move(Animal animal) {
        System.out.println(animal.color + " colored animal is flying with " + animal.legs + " wings");
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Whilst you've provided an alternative implementation, you don't appear to have reviewed the existing code. You also haven't explained why you've deviated from the assignment brief (no leg_number property). Whilst not following the interviewers instructions can be the correct choice, they would usually expect a rationale as to why you've decided not to. \$\endgroup\$
    – forsvarir
    Oct 27 at 21:48
  • \$\begingroup\$ I have not added review remarks as they have been already made by others. I wanted to provide an alternative to the pure inheritance approach, which is often a bad idea. Getting to grips with a strategy pattern might be beneficial for OP. However, the variables "leg_number" and "wing_number" are wrong in regards to the assignment. In Java-lingo I would argue that underscored variable names are not a practice that one should pick up. Consequently, the vars should be named "wingNumber" and "legNumber". As we can see from the type (int), the pseudo-suffix (number) should not even be needed. \$\endgroup\$
    – easyDaMan
    Oct 28 at 11:26
0
\$\begingroup\$

This is not a bad overall attempt, but there are some issues you should pick up further. I'm going to avoid giving you the straight answer where possible, as the true value is found in finding the answers yourself:

  • Not every cat has 4 legs. Not every bug has 6 legs. If my cat only has 3 legs, how could this code facilitate my cat, who should be walking on 3 legs? It seems safest to not hardcode this number. Same thing applies to the color of the animals.
    • Interestingly, you've already fixed it for the number of wings a bird has. The same solution applies for legs and color too.
  • Rather than have each subtype set all of its fields, you should consider making the setting of the Animal fields reusable. Instead of each subtype doing the same thing, how about you delegate this job down to the Animal constructor itself.
  • The bird's move method correctly allows for the "flying" message to be given instead. However, when you don't want to return the "flying" message, you've hardcoded the same response that your Animal class already implemented. Instead of hardcoding this message a second time, try to find out if there is a way that the Bird.move method is able to just reuse the base Animal.move output (when the bird does not have sufficient wings).
    • This solution entails having a proper override, so make sure that Bird.move overrides Animal.move before you tackle this.
  • After all these things are tackled, reconsider the access modifiers you use on leg_number, color and wing_number.
    • It should be private when you want no one other than the class itself to make use of it. You should use this as the default option. Only use the other access modifiers when you have a genuine need for them. Try to start by keeping everything private, and whenever you feel like something shouldn't be private, consider if you need to open this up to others or not.
    • It should only be protected if subtypes should be able to make use of it, but not other external code.
    • It should only be public when external code should be able to make use of it.

Note that if you want to test if your inheritance was properly implemented, you should try to work with your objects by using the base class:

Animal myCat   = new Cat();    
Animal myBug   = new Bug();
Animal myBird1 = new Bird(2);
Animal myBird2 = new Bird(0);

If you did everything right, your code should behave the same way (and compile, for starters) when you change all these variable types to Animal. If your code does not behave the same way, then you've not properly implemented your inheritance.


As an aside, unrelated to your actual code, this exercise sets you up for some bad coding standards. leg_number and wing_number should be legCount and wingCount, in keeping with the naming conventions.

"number of legs" would be correct English, but "leg count" is more idiomatic to a programmer and shout therefore be favored.

"leg number" doesn't mean what this exercise think it does. A "leg number" is the number given to a specific leg, e.g. "this is leg 5" (similar to "player 1", "aisle 5", "Apollo 13"). The number does not denote a total amount, it denotes a specific number given to a specific instance. There can be more players, more aisles, and more Apollo missions than the number indicates.

\$\endgroup\$

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.