3
\$\begingroup\$

I asked this on Stack Overflow and got some good ideas, but was advised to post here instead. There are obviously several ways to code this, each with a drawback and a benefit.

A person works a number of hours and can bill for the work in one of three ways:

  1. Hourly: the number of hours worked * hourly rate

  2. Dual: (for simplicity sake) first rate + second rate

  3. Flat: just an amount, regardless of the hours worked

Here's the code that I came up with, but I just can't help but think that there's a much more elegant way to do this. In the code below I have a class per rate type, one base class for a rate, a person, and a calculator that does the math. Suggestions?

using System;

public class Program
{
    public static void Main()
    {
        Person p = new Person();
        p.HoursWorked = 2;
        p.Rate = new Hourly(10);
        //p.Rate = new Dual(10, 1);
        //p.Rate = new Flat(10);
        Console.WriteLine(Calculator.Calculate(p));
    }
}



public class Person
{
    public RateStyle Rate { get; set; }
    public int HoursWorked { get; set; }
}

public abstract class RateStyle 
{
}

public class Dual : RateStyle
{
    public int First { get; private set; }
    public int Second { get; private set; }

    public Dual(int a, int b)
    {
        First = a;
        Second = b;
    }
}

public class Hourly : RateStyle
{
    public int Rate { get; private set; }
    public Hourly(int r)
    {
        Rate = r;
    }
}

public class Flat : RateStyle
{
    public int Rate { get; private set; }
    public Flat(int r)
    {
        Rate = r;
    }
}

public static class Calculator
{
    public static int Calculate(Person p)
    {
        if (p.Rate is Hourly)
        {
            return p.HoursWorked * ((Hourly)p.Rate).Rate;
        }
        if (p.Rate is Dual)
        {
            return ((Dual)p.Rate).First + ((Dual)p.Rate).Second;
        }

        return ((Flat)p.Rate).Rate;
    }
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Every question is implicitly about Best way to structure this code - you should write in the title what the code does instead. \$\endgroup\$ – t3chb0t Sep 2 '16 at 6:48
  • \$\begingroup\$ The OP and all the answers are way over-engineered for the stated problem. Why can't there be a single calculation involving all properties on one line of code? Person class contains all the involved properties and each instance will have values consistent w/ the "rate style". If a person is "dual" then the HourlyRate is zero for example. Put the calculation in the Person class and throw away everything else. You might want a property that says what "rate style" they are, but there is not enough "stuff" to justify abstract and sub classes. \$\endgroup\$ – radarbob Sep 6 '16 at 4:32
7
\$\begingroup\$

OO or not

The first thing to do is decide whether or not you want an object-oriented design for this problem. From the requirements you've described, there's a very simple solution that doesn't need any real design work at all:

public static int CalculateFee(int flatRate, int hourlyRate, int hours)
{
    return flatRate + hourlyRate*hours;
}

(The two parts of a "dual rate" would just be summed before being passed in, and either flatRate or hourlyRate could be 0)

You can then carry around those variables however is most convenient for you. For example maybe you'd put the rates together into some property-bag class:

public class Rate
{
    public int Hourly { get; set; }
    public int Flat { get; set; }
}

Or maybe you'd just pass them around as integers. Likewise whether CalculateFee was a public method somewhere or a private method, or even in-lined in another method, is really just a choice about how to organize things to read most clearly, rather than about design.


I think the above should be your default starting point. Building a design means making decisions, and every decision is a potential wrong decision, which becomes technical debt. So you should never build more or earlier than you're driven to by your requirements.

But there are some reasons you may immediately want a heavier design than this. For example:

  • You know- or think it's very likely- that you'll have to add new types of rates in addition to flat, dual and hourly.
  • There may be more functionality that you need associated with each rate, like wanting to be able to write a descriptive string ("A rate of 30$ per hour").
  • You want to be able to replace a rate with a test double for unit testing (I'm not sure that's really appropriate with these classes, it's hard to know without seeing how they're used)

Polymorphism

I won't write out a full OO version because Dmitry Nogin's answer already demonstrates this really well.

But to distill out the key point from that, if you're going to use an OO solution, you want to use polymorphism. If you're doing code like:

IThing thing;

if(thing is ThingTypeA)
{
    CalculationA(thing);
}
else if(thing is ThingTypeB)
{
    CalculationB(thing);
}
else if...

Then that's a pretty good sign that you should instead have a Calculation method on IThing, with a different implementation for ThingTypeA and ThingTypeB. That's a core part of OO design, and one that's easy to apply here (as Dmitry's answer shows)

Note that this also lets you encapsulate your data- those integer fields can be private to the Rate implementations. Your classes aren't just bags to hold data for anyone to read, they provide useful behaviour.

\$\endgroup\$
2
\$\begingroup\$

This design potentially allows to work with irregular hours.

How to use:

// 80 hours payslip
var payslip = new Payslip(80); 

// flat rate
Console.WriteLine(new FlatRate(1000).Apply(payslip)); 

// hourly rate
Console.WriteLine(new HourlyRate(15).Apply(payslip)); 

// flat + hourly rate
Console.WriteLine((new FlatRate(10) + new HourlyRate(15)).Apply(payslip)); 

Where:

public class Payslip
{
    public Payslip(decimal hours)
        : this(hours, 0)
    {
    }

    Payslip(decimal hours, decimal amount)
    {
        Hours = hours;
        Amount = amount;
    }

    public decimal Hours { get; }
    public decimal Amount { get; }
    public Payslip Pay(decimal amount) =>
        new Payslip(Hours, Amount + amount);

    public override string ToString() => 
        $"Pay ${Amount} for {Hours} hours.";
}

And:

public abstract class Rate
{
    public abstract Payslip Apply(Payslip payslip);
    public static Rate operator +(Rate left, Rate right) =>
        new DualRate(left, right);
}

And:

public class DualRate : Rate
{
    IEnumerable<Rate> Rates { get; }

    public DualRate(params Rate[] rates)
    {
        Rates = rates;
    }

    public override Payslip Apply(Payslip payslip) =>
        Rates.Aggregate(payslip, (p, r) => r.Apply(p)); 
}

And:

public class FlatRate : Rate
{
    decimal Value { get; }

    public FlatRate(decimal value)
    {
        Value = value;
    }

    public override Payslip Apply(Payslip payslip) =>
        payslip.Pay(Value);        
}

And:

public class HourlyRate : Rate
{
    decimal Value { get; }       

    public HourlyRate(decimal value)
    {
        Value = value;
    }

    public override Payslip Apply(Payslip payslip) =>
        payslip.Pay(Value * payslip.Hours);
}
\$\endgroup\$
  • \$\begingroup\$ I think it should work the other way round that you apply the rate to the playslip. \$\endgroup\$ – t3chb0t Sep 2 '16 at 6:46
  • \$\begingroup\$ @t3chb0t This way we can have everything immutable. \$\endgroup\$ – Dmitry Nogin Sep 2 '16 at 6:51
  • \$\begingroup\$ Ok, I could live with this if Apply was called ApplyTo ;-P it would better show that I apply the rate to the payslip and not the payslip to the rate. \$\endgroup\$ – t3chb0t Sep 2 '16 at 6:54
  • \$\begingroup\$ @t3chb0t Yep, sounds very reasonable, thanks. \$\endgroup\$ – Dmitry Nogin Sep 2 '16 at 6:56
2
\$\begingroup\$

You should probably get rid of this:

public static class Calculator
{
    public static int Calculate(Person p)
    {
        if (p.Rate is Hourly)
        {
            return p.HoursWorked * ((Hourly)p.Rate).Rate;
        }
        if (p.Rate is Dual)
        {
            return ((Dual)p.Rate).First + ((Dual)p.Rate).Second;
        }

        return ((Flat)p.Rate).Rate;
    }
}

The whole point of all these subclasses is to take advantage of polymorphism. Instead of writing if statements, we should seek to create methods instead. (See Google's Clean Code Talks for more.)

Instead, create Calculate() methods inside your RateStyle (I renamed this to Rate) subclasses:

public abstract class Rate 
{
    public virtual int Calculate(Person p);
}

public class DualRate : Rate
{
    public override int Calculate(Person p) => First + Second;
}

public class HourlyRate : Rate
{
    public override int Calculate(Person p) => p.HoursWorked * Rate;
}

public class FlatRate : Rate
{
    public override int Calculate(Person p) => Rate;
}

Now, you may implement a Bill() method inside Person:

public class Person
{
    public int Bill() => Rate.Calculate(this);
}
\$\endgroup\$
2
\$\begingroup\$

Most of the improvements have been indicated by other reviewers and there is no need to repeat what they have said but in addition

Should the class containing Rate and HoursWorked be called person?

No, this class should have a more meaningful name i.e BillingInformation as this class contains billing information.

How can you make Person class more efficient?

Replace class with Struct. A struct type is a value type that is typically used to encapsulate small groups of related variables. Structs can also contain constructors, constants, fields, methods, properties, indexers, operators, events, and nested types, although if several such members are required, you should consider making your type a class instead. An example will be

public struct BillingInformation
{
    public RateStyle Rate { get; set; }
    public int HoursWorked { get; set; }
}

public static class Calculator
{
    public static int Calculate(BillingInformation p)
    {
        if (p.Rate is Hourly)
        {
            return p.HoursWorked * ((Hourly)p.Rate).Rate;
        }
        if (p.Rate is Dual)
        {
            return ((Dual)p.Rate).First + ((Dual)p.Rate).Second;
        }

        return ((Flat)p.Rate).Rate;
    }
}

class Program
    {
        public static void Main(string[] args)
        {
            //   Person p = new Person();
            // p.HoursWorked = 2;
            //  p.Rate = new Hourly(10);
            //p.Rate = new Dual(10, 1);
            //p.Rate = new Flat(10);

            BillingInformation billing = new BillingInformation();
            billing.HoursWorked = 2;
            billing.Rate = new Hourly(10);
            Console.WriteLine(Calculator.Calculate(billing));
            Console.ReadKey(true);
        }
    }

What SOLID principle are you breaking ?

Liskov substitution. Even though, all those classes are inheriting from RateStyle the subclasses/ derived classes behave entirely different from it.

what is wrong with your abstract class?

It just an empty class with no class members. When creating an abstract class, there should be a similarity between the deriving classes that could serve as methods, class variables. Example of this, Rate and Calculate() should have been marked as abstract. So that the implementation of calculate will be derived class specific. Hence, the Calculator class will be redundant in this case.

What should you have avoided?

I'm not sure why you marked the Calculator class has static, this should have been an instance method. I tend to limit the use of static to create extension methods and create Singleton properties and variables. There also more substantial reasons to use them.

I hope this helps.

\$\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.