35

Although this could be a programming language agnostic question, I'm interested in answers targeting the .NET ecosystem.

This is the scenario: suppose we need to develop a simple console application for the public administration. The application is about vehicle tax. They (only) have the following business rules:

1.a) If the vehicle is a car and the last time its owner paid the tax was 30 days ago then the owner has to pay again.

1.b) If the vehicle is a motorbike and the last time its owner paid the tax was 60 days ago then the owner has to pay again.

In other words, if you have a car you have to pay every 30 days or if you have a motorbike you have to pay every 60 days.

For each vehicle in the system, the application should test those rules and print those vehicles (plate number and owner information) that don't satisfy them.

What I want is:

2.a) Conform to the S.O.L.I.D. principles (especially Open/closed principle).

What I don't want is (I think):

2.b) An anemic domain, therefore business logic should go inside business entities.

I started with this:

public class Person
// You wanted a banana but what you got was a gorilla holding the banana and the entire jungle.
{
    public string Name { get; set; }

    public string Surname { get; set; }
}

public abstract class Vehicle
{
    public string PlateNumber { get; set; }

    public Person Owner { get; set; }

    public DateTime LastPaidTime { get; set; }

    public abstract bool HasToPay();
}

public class Car : Vehicle
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 30;
    }
}

public class Motorbike : Vehicle
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 60;
    }
}

public class PublicAdministration
{
    public IEnumerable<Vehicle> GetVehiclesThatHaveToPay()
    {
        return this.GetAllVehicles().Where(vehicle => vehicle.HasToPay());
    }

    private IEnumerable<Vehicle> GetAllVehicles()
    {
        throw new NotImplementedException();
    }
}

class Program
{
    static void Main(string[] args)
    {
        PublicAdministration administration = new PublicAdministration();
        foreach (var vehicle in administration.GetVehiclesThatHaveToPay())
        {
            Console.WriteLine("Plate number: {0}\tOwner: {1}, {2}", vehicle.PlateNumber, vehicle.Owner.Surname, vehicle.Owner.Name);
        }
    }
}

2.a: The Open/closed principle is guaranteed; if they want bicycle tax you just inherit from Vehicle, then you override the HasToPay method and you're done. Open/closed principle satisfied through simple inheritance.

The "problems" are:

3.a) Why a vehicle has to know if it has to pay? Isn't a PublicAdministration concern? If public administration rules for car tax change, why Car has to change? I think you should be asking: "then why you put a HasToPay method inside Vehicle?", and the answer is: because I don't want to test for the vehicle type (typeof) inside PublicAdministration. Do you see a better alternative?

3.b) Maybe tax payment is a vehicle concern after all, or maybe my initial Person-Vehicle-Car-Motorbike-PublicAdministration design is just plain wrong, or I need a philosopher and a better business analyst. A solution could be: move the HasToPay method to another class, we can call it TaxPayment. Then we create two TaxPayment derived classes; CarTaxPayment and MotorbikeTaxPayment. Then we add an abstract Payment property (of type TaxPayment) to the Vehicle class and we return the right TaxPayment instance from Car and Motorbike classes:

public abstract class TaxPayment
{
    public abstract bool HasToPay();
}

public class CarTaxPayment : TaxPayment
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 30;
    }
}

public class MotorbikeTaxPayment : TaxPayment
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 60;
    }
}

public abstract class Vehicle
{
    public string PlateNumber { get; set; }

    public Person Owner { get; set; }

    public DateTime LastPaidTime { get; set; }

    public abstract TaxPayment Payment { get; }
}

public class Car : Vehicle
{
    private CarTaxPayment payment = new CarTaxPayment();
    public override TaxPayment Payment
    {
        get { return this.payment; }
    }
}

public class Motorbike : Vehicle
{
    private MotorbikeTaxPayment payment = new MotorbikeTaxPayment();
    public override TaxPayment Payment
    {
        get { return this.payment; }
    }
}

And we invoke the old process this way:

    public IEnumerable<Vehicle> GetVehiclesThatHaveToPay()
    {
        return this.GetAllVehicles().Where(vehicle => vehicle.Payment.HasToPay());
    }

But now this code won't compile because there's no LastPaidTime member inside CarTaxPayment/MotorbikeTaxPayment/TaxPayment. I'm starting to see CarTaxPayment and MotorbikeTaxPayment more like "algorithm implementations" rather than business entities. Somehow now those "algorithms" need the LastPaidTime value. Sure, we can pass the value to the constructor of TaxPayment, but that would violate vehicle encapsulation, or responsibilities, or whatever those OOP evangelists call it, wouldn't it?

3.c) Suppose we already have an Entity Framework ObjectContext (which uses our domain objects; Person, Vehicle, Car, Motorbike, PublicAdministration). How would you do to get an ObjectContext reference from PublicAdministration.GetAllVehicles method and therefore implement the funcionality?

3.d) If we also need the same ObjectContext reference for CarTaxPayment.HasToPay but a different one for MotorbikeTaxPayment.HasToPay, who is in charge of "injecting" or how would you pass those references to the payment classes? In this case, avoiding an anemic domain (and thus no service objects), don't lead us to disaster?

What are your design choices for this simple scenario? Clearly the examples I've shown are "too complex" for an easy task, but at the same time the idea is to adhere to the S.O.L.I.D. principles and prevent anemic domains (of which have been commissioned to destroy).

0

8 Answers 8

16

I would say that neither a person nor a vehicle should know whether a payment is due.

reexamining the problem domain

If you look at the problem domain "people having cars": In order to purchase a car you have some sort of contract which transfers ownership from one person to the other, neither the car nor the person change in that process. Your model is completely lacking that.

thought experiment

Assuming there is a married couple, the man owns the car and he is paying the taxes. Now the man dies, his wife inherits the car and will pay the taxes. Yet, your plates will stay the same (at least in my country they will). It is still the same car! You should be able to model that in your domain.

=> Ownership

A car that is not owned by anyone is still a car but nobody will pay taxes for it. In fact, you are not paying taxes for the car but for the privilege of owning a car. So my proposition would be:

public class Person
{
    public string Name { get; set; }

    public string Surname { get; set; }

    public List<Ownership> getOwnerships();

    public List<Vehicle> getVehiclesOwned();
}

public abstract class Vehicle
{
    public string PlateNumber { get; set; }

    public Ownership currentOwnership { get; set; }

}

public class Car : Vehicle {}

public class Motorbike : Vehicle {}

public abstract class Ownership
{
    public Ownership(Vehicle vehicle, Owner owner);

    public Vehicle Vehicle { get;}

    public Owner CurrentOwner { get; set; }

    public abstract bool HasToPay();

    public DateTime LastPaidTime { get; set; }

   //Could model things like payment history or owner history here as well
}

public class CarOwnership : Ownership
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 30;
    }
}

public class MotorbikeOwnership : Ownership
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 60;
    }
}

public class PublicAdministration
{
    public IEnumerable<Ownership> GetVehiclesThatHaveToPay()
    {
        return this.GetAllOwnerships().Where(HasToPay());
    }

}

This is far from perfect and probably not even remotely correct C# code but I hope you get the idea (and somebody could clean this up). The only downside is that you would probably need a factory or something to create the correct CarOwnership between a Car and a Person

2
  • I think the vehicle itself should record the taxes paid for it, and individuals should record taxes paid for all their vehicles. A tax code could be written so that if the tax on a car is paid June 1 and it is sold on June 10, the new owner might become liable for the tax on June 10, July 1, or July 10 (and even if it's presently one way it could change). If the 30-day rule is constant for the car even through ownership changes, but cars that are owed by nobody can't pay taxes, then I would suggest that when a car which had been unowned for 30 or more days is purchased, a tax payment...
    – supercat
    Apr 7, 2015 at 17:31
  • ...get recorded from a fictitious "Unowned vehicle tax credit" person, so that as of the time of sale the tax liability will be either zero or thirty days, regardless of how long the vehicle had been unowned.
    – supercat
    Apr 7, 2015 at 17:32
4

I think that in this sort of situation, where you want the business rules to exist outside of the target objects, testing for type is more than acceptable.

Certainly, in many situations you should use a Strategy patern and let the objects handle things themselves, but that's not always desireable.

In the real world, a clerk would look at the type of vehicle and lookup its tax data based on that. Why shouldn't your software follow the same busienss rule?

3
  • Ok, say you convinced me and from GetVehiclesThatHaveToPay method we inspect the vehicle type. Who should be responsible for the LastPaidTime? Is LastPaidTime a Vehicle concern or a PublicAdministration one? Because if it's a Vehicle, shouldn't that business logic reside in Vehicle? What can you tell me about question 3.c?
    – David Robert Jones
    Feb 1, 2012 at 23:01
  • @DavidRobertJones - Ideally, I would lookup the last payment in the Payment history log, based on vehicle license. A tax payment is a tax concern, not a vehicle concern. Feb 1, 2012 at 23:04
  • 6
    @DavidRobertJones I think you are confusing yourself with your own metaphor. What you have is not an actual vehicle that needs to do all the things a vehicle does. Instead, you have named, for simplicity a TaxableVehicle (or something similar) a Vehicle. Your entire domain is tax payments. Don't worry about what actual vehicles do. And, if necessary drop the metaphor and come up with a more appropriate one.
    – Tim
    Feb 1, 2012 at 23:25
3

What I don't want is (I think):

2.b) An anemic domain, which means business logic inside business entities.

You have this backwards. An anemic domain is one which the logic is outside the business entities. There are lots of reasons why using additional classes to implement the logic is a bad idea; and only a couple for why you should do it.

Hence the reason why it's called anemic: the class doesn't have anything in it..

What I would do:

  1. Change your abstract Vehicle class to be an interface.
  2. Add an ITaxPayment interface that the vehicles must implement. Have your HasToPay hang off of here.
  3. Remove the MotorbikeTaxPayment, CarTaxPayment, TaxPayment classes. They are unnecessary complications/clutter.
  4. Each vehicle type (car, bike, motorhome) should implement at a minimum Vehicle and, if they are taxed, should implement ITaxPayment as well. This way you can delineate between those vehicles that aren't taxed and those that are... Assuming this situation even exists.
  5. Each vehicle type should implement, within the boundaries of the class itself, the methods defined in your interfaces.
  6. Also, add last payment date to your ITaxPayment interface.
1
  • You're right. Originally the question wasn't formulated that way, I deleted a part and the meaning changed. It's fixed now. Thanks!
    – David Robert Jones
    Feb 2, 2012 at 0:44
2

Why a vehicle has to know if it has to pay? Isn't a PublicAdministration concern?

No. As I understand it your whole app is about taxing. So the concern about whether a vehicle should be taxed is no more a concern of PublicAdministration then anything else in the entire program. There is no reason to put it anywhere else but here.

public class Car : Vehicle
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 30;
    }
}

public class Motorbike : Vehicle
{
    public override bool HasToPay()
    {
        return (DateTime.Today - this.LastPaidTime).TotalDays >= 60;
    }
}

These two code snippets differ only by the constant. Instead of overriding the whole HasToPay() function override an int DaysBetweenPayments() function. Call that instead of using the constant. If this is all they actually differ on, I'd drop the classes.

I'd also suggest that Motorbike/Car should be a category property of vehicle rather then subclasses. That gives you more flexibility. For example, it makes it easy to change the category of a motorbike or car. Of course bikes are unlikely to be transformed into cars in real life. But you know that a vehicle going to get entered wrong and need to be changed later.

Sure, we can pass the value to the constructor of TaxPayment, but that would violate vehicle encapsulation, or responsibilities, or whatever those OOP evangelists call it, wouldn't it?

Not really. An object that deliberately passes its data to another object as a parameter isn't a large encapsulation concern. The real concern is other objects reaching inside this object to pull data out or manipulating the data inside the object.

1

You might be on the right track. The problem is, as you've noticed, that there's no LastPaidTime member inside TaxPayment. That's exactly where it belongs, although it doesn't need to be publicly exposed at all:

public interface ITaxPayment
{
    // Your base TaxPayment class will know the 
    // next due date. But you can easily create
    // one which uses (for example) fixed dates
    bool IsPaymentDue();
}

public interface IVehicle
{
    string Plate { get; }
    Person Owner { get; }
    ITaxPayment LastTaxPayment { get; }
} 

Each time you receive a payment, you create a new instance of a ITaxPayment according to the current policies, which can have completely different rules than the previous one.

1
  • The thing is that the payment due depend on the last time the owner paid (different vehicles, different due dates). How ITaxPayment know which was the last time a vehicle (or owner) paid in order to do the calculation?
    – David Robert Jones
    Feb 1, 2012 at 23:11
1

I agree with the answer by @sebastiangeiger that the problem is really about vehicle ownerships rather than vehicles. I am going to suggest using his answer but with a few changes. I would make the Ownership class a generic class:

public class Vehicle {}

public abstract class Ownership<T>
{
    public T Item { get; set; }

    public DateTime LastPaidTime { get; set; }

    public abstract TimeSpan PaymentInterval { get; }

    public virtual bool HasToPay
    {
        get { return (DateTime.Today - this.LastPaidTime) >= this.PaymentInterval; }
    }
}

And use inheritance to specify the payment interval for each vehicle type. I also suggest using the strongly-typed TimeSpan class instead of plain integers:

public class CarOwnership : Ownership<Vehicle>
{
    public override TimeSpan PaymentInterval
    {
        get { return new TimeSpan(30, 0, 0, 0); }
    }
}

public class MotorbikeOwnership : Ownership<Vehicle>
{
    public override TimeSpan PaymentInterval
    {
        get { return new TimeSpan(60, 0, 0, 0); }
    }
}

Now your actual code only needs to deal with one class - Ownership<Vehicle>.

public class PublicAdministration
{
    List<Ownership<Vehicle>> VehicleOwnerships { get; set; }

    public IEnumerable<Vehicle> GetVehiclesThatHaveToPay()
    {
        return VehicleOwnerships.Where(x => x.HasToPay).Select(x => x.Item);
    }
}
0
0

I hate to break it to you, but you have an anemic domain, i.e. business logic outside of objects. If this is what you are going for that is okay, but you should read about reasons to avoid one.

Try not to model the problem domain, but model the solution. That is why you are ending up with parallel inheritance hierarchies and more complexity than you need.

Something like this is all you need for that example:

public class Vehicle
{
    public Boolean isMotorBike()
    public string PlateNumber { get; set; }
    public String Owner { get; set; }
    public DateTime LastPaidTime { get; set; }
}

public class TaxPaymentCalculator
{
    public List<Vehicle> GetVehiclesThatHaveToPay(List<Vehicle> all)
}

If you want to follow SOLID that is great, but as Uncle Bob said himself, they are just engineering principles. They are not meant to be stringently applied, but are guidelines that should be considered.

6
  • 4
    I think your solution is not particularly scalable. You would need to add a new boolean for each type of vehicle you add. That's a poor choice in my opinion. Feb 1, 2012 at 22:40
  • @Garrett Hall, I liked you removed the Person class from my "domain". I wouldn't have that class in the first place, so sorry about that. But isMotorBike doesn't make sense. Which would be the implementation?
    – David Robert Jones
    Feb 1, 2012 at 22:45
  • 1
    @DavidRobertJones: I agree with Mystere, adding a isMotorBike method is not a good OOP design choice. It's like replacing polymorphism with a type code (albeit a boolean one).
    – vgru
    Feb 1, 2012 at 22:59
  • @MystereMan, one could easily drop the bools and use an enum Vehicles
    – radarbob
    Dec 24, 2013 at 3:11
  • +1. Try not to model the problem domain, but model the solution. Pithy. Memorably said. AND the principles comment. I see too many attempts at strict literal interpretation; nonsense.
    – radarbob
    Dec 24, 2013 at 3:20
0

I think you're right that the payment period is not a property of the actual Car/Motorbike. But it is an attribute of the class of vehicle.

While you could go down the road of having an if/select on the Type of object, this is not very scalable. If you later add lorry and Segway and push bike and bus and aeroplane (may seem unlikely, but you never know with public services) then the condition becomes larger. And if you want to change the rules for a lorry, you have to find it in that list.

So why not make it, quite literally, an attribute and use reflection to pull it out? Something like this:

[DaysBetweenPayments(30)]
public class Car : Vehicle
{
    // actual properties of the physical vehicle
}

[AttributeUsage(AttributeTargets.Class, Inherited = false)]
public class DaysBetweenPaymentsAttribute : Attribute, IPaymentRequiredCalculator
{
    private int _days;

    public DaysBetweenPaymentAttribute(int days)
    {
        _days = days;
    }

    public bool HasToPay(Vehicle vehicle)
    {
        return vehicle.LastPaid.AddDays(_days) <= DateTime.Now;
    }
}

You could also go with a static variable, if that is more comfortable.

The downsides are

  • that it will be slightly slower, and I'm assuming you have to process a lot of vehicles. If that is an issue then I might take a more pragmatic view and stick with the abstract property or interfaced approach. (Although, I'd consider caching by type too.)

  • You will have to validate at startup that each Vehicle subclass has an IPaymentRequiredCalculator attribute (or allow a default), because you don't want it to process 1000 cars, only to crash because Motorbike doesn't have a PaymentPeriod.

The upside is that if you later come across a calendar month or annual payment, you can just write a new attribute class. This is the very definition of OCP.

4
  • The major issue with this is that if you want to change the number of days between payments for a Vehicle you will need to rebuild and redeploy, and if the payment frequency varies based on a property of the entity (e.g. engine size) it will be difficult to get an elegant fix for that.
    – Ed James
    Feb 2, 2012 at 10:22
  • @EdWoodcock: I think the first issue is true of most solutions and it really wouldn't worry me. If they want that level of flexibility later, I'd give them a DB app. On the second point, I completely disagree. At that point, you just add vehicle as an optional argument to HasToPay() and ignore it where you don't need it.
    – pdr
    Feb 2, 2012 at 10:42
  • I guess it depends on the long-term plans you have for the app, plus the particular requirements of the client, as to whether it's worth plugging in a DB if none already exists (I do tend to assume that pretty much all software is DB-driven, which I know is not always the case). However, on the second point, the important qualifier is elegant; I would not say that adding an extra parameter to a method on an attribute that then takes an instance of the class that the attribute is attached to is a particularly elegant solution. But, again, it depends on your client requirements.
    – Ed James
    Feb 2, 2012 at 10:50
  • @EdWoodcock: Actually, I was just thinking about this and the lastPaid argument is already going to have to be a property of Vehicle. With your comment in mind, it might be worth replacing that argument now, rather than later - will edit.
    – pdr
    Feb 2, 2012 at 10:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.