4

I am currently facing the following scenario:

  • When defining the requirements of my system on a piece of paper, I described two different entities A and B (e.g. Cat and Dog)
  • After listing what A and B should do, they ended up having the exact same behavior requirements (e.g. the behavior of an Animal).
  • To reflect that, I implemented A and B as classes and both inheriting from C (e.g. the abstract class Animal).

Should A and B be kept as classes or should they be removed from the architecture altogether, only keeping C? Is this a sign of a problem in the architecture of this solution?


Example:

Suppose I am implementing a pet shop system and I have the following requirements:

  • My pet shop can groom cats and dogs
  • When a cat is groomed the dead fur count should be zero
  • When a dog is groomed the dead fur count should be zero

To solve this problem I implement the following tests:

[Test]
public void When_DogIsGroomed_Should_DeadFurCountBeZero()
{
    PetShop petShop = new PetShop();
    Dog dog = new Dog();
    petShop.Groom(dog);
    Assert.AreEqual(0, dog.DeadFurCount);
}

[Test]
public void When_CatIsGroomed_Should_DeadFurCountBeZero()
{
    PetShop petShop = new PetShop();
    Cat cat = new Cat();
    petShop.Groom(cat);
    Assert.AreEqual(0, cat.DeadFurCount);
}

With the following implementation code:

public abstract class Animal()
{
    public int DeadFurCount { get; private set; }

    internal void Groom()
    {
        this.DeadFurCount = 0;
    }
}
public class PetShop()
{
    public void Groom(Animal animal)
    {
        animal.Groom();
    }
}
public class Dog(): Animal
{

}
public class Cat(): Animal
{

}

I this example, should Cat and Dog be removed, thus only remaining with Animal for the solution final architecture? Are those empty classes a bad sign or design?

One thing I fear about removing is that my tests would be less representative of the system I am implementing. After all, my requirements talk about dogs and cats, not animals. But at the same time, empty classes are usually a smell.

  • You could just add factory methods that return animals. Animal getDog() { return new Animal(); }, in case the behaviour of Dog and Cat deviates in the future you can replace Animal with a subclass. – Helena Nov 27 '19 at 20:35
  • You don't ever intend to override the Groom() method in your derived classes? – Robert Harvey Nov 27 '19 at 20:35
  • Is the possibility of ever overriding the Groom method the condition for deciding that his is a good or bad design for now? I do not necessarily know for now if the requirement for a different Groom behavior for cats and dogs will appear as the PetShop grows. Perhaps, cats should keep their belly intact? I don't know for now. In that sense, that possibility would be a valid scenario regarding what I am asking. – Albuquerque Nov 27 '19 at 20:38
14

After all, my requirements talk about dogs and cats, not animals. But at the same time, empty classes are usually a smell.

Have a discussion with your domain experts about why they specifically use the words "dog" and "cat" instead of a more generic word like "animal" or "pet". I can think of a few possible outcomes:

  • Dogs and cats are treated exactly the same in your domain: Simplify your vocabulary and use a single term (and therefore a single class).
  • Certain business rules depend on the type of animal: Use a single class and model the type of animal as an enum. Inheritance isn't the best solution here since it's likely not the responsibility of Animal to cater to all present and future business rules.
  • There is something fundamentally different about the structure of dogs and cats: Go ahead and model them as two separate entities (possibly with a common superclass if you need a shared structure).
| improve this answer | |
  • Note that for the third bullet point, future expectations can be enough justification to keep them separated. Just because they are the same today does not mean they will always be the same. What happens if tomorrow you want to give Cat a throwUpHairBall() method? Are you going to be happy having to again re-separate your now merged Animal class? – Flater Nov 29 '19 at 8:53
  • @Flater: "Future expectations" sounds like YAGNI to me. Would you also create subtypes for different breeds of cat because they "might be required in the future"? If the software is well-designed, it should be easy enough to add Cat when it's actually required. – casablanca Nov 29 '19 at 18:21
  • If you consider a reasonable expectation to violate YAGNI, then I think you're being too extreme with YAGNI. – Flater Nov 30 '19 at 0:40
  • You seem to have ignored the thought exercise in my previous comment. By your own argument, if you create a Cat class right now, why not create throwUpHairBall() right now as well since you might need it tomorrow? How far do you go with that? That's the basis of YAGNI - even if you're very sure you'll need it tomorrow, just implement it tomorrow, it won't take any longer. – casablanca Nov 30 '19 at 7:52
  • You seem to have missed the point of reasonable expectations. That doesn't mean "anything and everything you can think of". – Flater Nov 30 '19 at 8:04
7

No, empty classes are not a smell. They're a sign that you know the value of a good name.

That's exactly what these empty classes have going for them. They're good names. I create empty classes to give exceptions good names all the time.

Dog and Cat might someday have different grooming logic but right now they don't. This is fine. It would also be fine to push that logic down if you felt the grooming was more likely to change independently. Just because it's the same right now doesn't mean you have to do it in one place. It's ok to have both x and y even when they're both set to 1.

Where you'll get into trouble is if you start imagining other animals that might someday be groomed and adding them 'just in case'. That will get you a beating from the YAGNI stick.

| improve this answer | |
  • 1
    Your answer is fine, but I guess you are not creating empty exception classes just because of their "nice name", but because of the possibility of catching them more fine-grained. – Doc Brown Nov 28 '19 at 12:12
  • 1
    @DocBrown not necessarily. I'm dead serious about the value of a good name. Even if the only good it does happens when a human is debugging. – candied_orange Nov 28 '19 at 13:45
  • I like good names too, but what does creating an empty class buy you that an exception message or an enum doesn't? Unless you want encapsulation and/or polymorphism, there are simpler language features that can achieve the same result. – casablanca Nov 29 '19 at 18:37
  • @casablanca the name is at a higher level of abstraction than the message. I might reuse a custom name in 5 places and use a unique dynamic message in all 5 places. Names are not just for the code that knows them. They're for the humans as well. The only problem I have with these empty classes is they could have been one liners. Makes it clear you didn't just forget to fill them in. – candied_orange Nov 29 '19 at 21:45
2

Yes its a code smell.

If you have empty classes with no logic, then it suggests that somewhere in your code you have conditional logic which checks what class an instance is.

eg.

if(x is Dog) then y

Also it might suggest that you have an ever increasing number of empty classes, Cat, Dog, Cow, Sheep etc etc

These kind of things should normally be replaced by an enum or string "Type" property on the parent object.

Of course I need to add the usual caveat. "code smell" != bad code. Its just something that is seen often in conjunction with bad code.

Also it suggests to me that you are applying an OOP approach to business rules, which isnt always a good idea.

Your pet grooming business example shouldnt have dog and cat objects with a groom method. I would expect it to have a Service class with no methods.

Service
{
    Id;
    CustomerId
    Type;
    Price;
    Date;
    Status;
}

A Customer would book a Service Type= "Dog SuperGroom" and you would have classes such as LateServiceEmailer where your OOP would come into affect.

No Business wants to hear "Oh we cant make that change because a Fish isnt an animal.

| improve this answer | |
0

There is an interesting perspective inherent to source code (the human-readable form, of course), one you need to keep in mind!

It is the abstraction of a mental process! Your mental process. The mental process that attempts to construct a conceptual representation, your conceptual representation. Your interpretation of a system, a part of the world (this, or an imaginary world).

An empty class with a name is not necessarily a sign of bad architecture. It is more like a "seal" of your architecture. This is what you saw when looking at your domain.

Why are not all cars white in the real world? Or black... or beige? Having a property (i.e. color) for an object, when it makes absolutely no difference to the functionality/use most of the time seems quite redundant. But clients are oftentimes ready to even make fuss about it, take pride in it, enjoy it (OK, not always, of course, but you get my point). It makes their life better, without adding true engineering value (again, in most cases). Keeping an extra Color property in a car database is just another pain to the developer, it increases the size of a database, it just comes and goes, it introduces no additional validation, etc.

When writing code, you need to keep the distinction in mind, between what has design implications and what is simply there for "completeness".

Oh, and, something else. You say:

I this example, should Cat and Dog be removed, thus only remaining with Animal for the solution final architecture? Are those empty classes a bad sign or design?

Yes. Remember, it's still today! Never take today's architecture for final! You need to ask yourself, what is the cost of having to introduce these classes in a few months, particularly after having built a relatively extensive code base, spreading dependencies around and potentially having gone through numerous assumptions? What if the requirements change, as they often do, let alone in "breaking" ways?

So, you can keep the classes there today, for the sake of being flexible enough to accommodate changes tomorrow. Foreseeing change and being prepared for it one of the things that makes a good developer. And don't forget, of course, that there is an art in keeping a good balance between over-engineering and under-engineering.

| improve this answer | |
0

Take a different example to illustrate the point with a more simple answer.

I have 2 measurements - HeightFromGroundInMeters and NumberOfPotatoes. They are both behaviorally integers (say), however semantically they are totally unrelated and certainly not logically interchangeable.

I'd say this was an example of classes that exist usefully (in the sense it's clear enough that they clearly represent two different things and logical equivalence is probably wrong) but they both have same behavioral properties.

TLDR: Perfectly legit.

| improve this answer | |

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.