8
\$\begingroup\$

Follow-up posted -> Follow up to Pay Rate Calculator


This little bit of code is just some code that I wrote from a flowchart at the beginning of a programming and logic book that I have added some to. I tried to create it as an object.

I would like a review on what I am doing and if I am doing it right, and what I could do better. I want you to look at this like I am a novice to intermediate Programmer. I don't think that I learned as much as I should have while I was in school, and it probably shows.

Main Method

public static void Main (string[] args)
{
    var payCheck = new PayRateCalculator();
    Console.Write ("Please provide the Employees Last Name >>");
    payCheck.lastName = Console.ReadLine();
    Console.Write ("How many hours has employee {0} worked? >>", payCheck.lastName);
    payCheck.hours = Convert.ToInt32 (Console.ReadLine());
    Console.Write ("What is employee {0}'s hourly wage? >>", payCheck.lastName);
    payCheck.payrate = Convert.ToInt32 (Console.ReadLine ());
    Console.WriteLine ("{0}'s Gross pay is ${1}", payCheck.lastName, payCheck.grossPay);
    Console.WriteLine ("{0}'s Net pay is ${1}", payCheck.lastName, payCheck.netPay);   
}

Class

class PayRateCalculator
{
    private float _payrate = 8.25f;
    private float _withholdingRate = 0.20F;

    public float hours { get; set; }
    public float grossPay { 
        get {
            return hours * _payrate;
        }
    }
    public float netPay { 
        get {
            return grossPay - (grossPay * _withholdingRate);
        }
    }
    public float payrate {
        get {
            return _payrate;
        }
        set {
            _payrate = value;
        }
    }
    public string lastName { get; set; }

    public PayRateCalculator (string lastName, float hours, float payRate)
    {
        this.hours = hours;
        this.payrate = payrate;
        this.lastName = lastName;
    }

    public PayRateCalculator ()
    {

    }
}

Just a simple console application. Help me to relearn coding the right way.

\$\endgroup\$
  • 1
    \$\begingroup\$ payCheck = PayRateCalculator should be enough to raise an eyebrow. \$\endgroup\$ – abuzittin gillifirca Dec 23 '13 at 10:03
  • \$\begingroup\$ @abuzittingillifirca, it started off as one thing and turned into another. the flowchart from the book was set up for step by step, which I could have done without creating an object. that is the reason for the difference in naming. thank you for pointing that out though. \$\endgroup\$ – Malachi Dec 23 '13 at 14:16
7
\$\begingroup\$

As a few people have pointed out, there is actually nothing really wrong with this code.

I mean, there needs to be some naming standardization, most notably any public property should be capital.

I would also advise a backing field for each property making the constructor a bit cleaner:

_hours = hours;
_payrate = payrate;
_lastName = lastName;

That being said from a design point of view, it is best to mirror your objects to real world objects.

Is a PayRateCalculator actually a PayCheck? Would it not make more sense to have a paycheck of some kind, once you have your details assigned you can return the paycheck as a separate thing?

The means with which you generate the paycheck should stand alone from the paycheck itself, allowing the possibility in future of having multiple calculation methods or doing something interesting with the paycheck object.

Although again, these are just design considerations as your application scales.

There is nothing inherently wrong with your code, which is well-written, decently-named and functional.

Well done.

\$\endgroup\$
6
\$\begingroup\$
  1. The standard naming convention for properties is PascalCase in C#.

  2. I try and give default "magic constants" a name. In this case I admit it's fairly obvious however you never really know what your code will turn into. It is also good to form habits so you wont forget these things.

    private const float DefaultPayrate = 8.25f;
    private const float DefaultWithholdingRate = 0.2f;
    
    private float _Payrate = DefaultPayRate;
    private float _WithholdingRate = DefaultWithholdingRate;
    
  3. You should use decimal as you are operating with currencies and pay rates. Floating point rounding can unexpectedly bite you.

  4. There is no way to change the _withholdingRate.

  5. As the PayRateCalculator only really makes sense once you have set the name, hours and payrate I'd consider removing the default constructor. Then you'd have to change the calling code to temporarily store these things before you can instantiate an instance. I'd also consider changing most of those properties to a private set. This way you can avoid getting a PayRateCalculator into an inconsistent state.

    Another programmer cannot easily figure out from just the class interface (ctor and public methods and properties) which he needs to populate in order to make sensible use of it. Taking the choice away (forcing to pass in the important properties through the ctor) makes that obvious.

\$\endgroup\$
4
\$\begingroup\$

From a design perspective

A domain consists of data and services and entities. Our minds should be clear where each class belongs in the above categories.

Data Objects

See:

var payCheck = ....
payCheck.hours = someVal; // what?
payCheck.hours = someOtherVal; // really?

This doesn't make sense because we know that IRL paychecks represent a fact: e.g. "John Doe was paid $100." They do not change after the fact. This is true for any print documents: receipts, tickets, etc.

Due to the fact that they are immutable, once they are created (i.e. composed) how did their components came to be is irrelevant. A data object should not have any creation logic, let alone the creation logic of it components. c.f. netPay, grossPay above.

Service Objects

Service objects are purely or heavily behavior focused components. Complementarily they have little state. When your use case is "do X to Y", X is prior to Y. When we try to "Calculate payment amount for some work". It is implied that there is a calculation method independent of the amount of work(DUH). More concisely: Service Objects [are|are like] a collection of functions. Enough philosophizing.

Instead of :

var calculator = ....

calculator.hours = x;
use(calculator.grossPay);

calculator.hours = y;
use(calculator.grossPay);

We should see:

var calculator = ....

use(calculator.grossPay(x));

use(calculator.grossPay(y));

Dividing PayRateCalculator

Dividing PayRateCalculator along this line we get:

class PayRateCalculator
{
    private decimal _withholdingRate;

    public PayRateCalculator (decimal withHoldingRate)
    {
        this._withholdingRate = withHoldingRate;
    }

    public PayCheck CreatePayCheck(string lastName, decimal hours, decimal payRate) {
        decimal grossPay = hours * payRate;
        decimal netPay = grossPay - (grossPay * _withholdingRate);
        return new PayCheck(lastName, netPay, grossPay);
    }
}

class PayCheck {
    public decimal GrossPay  {get; private set;}
    public decimal NetPay  {get; private set;}
    public string LastName {get; private set;}

    public PayCheck(string lastName, decimal netPay, decimal grossPay) {
        this.LastName = lastName;
        this.GrossPay = grossPay;
        this.NetPay = netPay;
    }
}

Remaining problems

We observe that PayRateCalculator do not want to do anything with lastName. IRL too, the effect of your last name on your paycheck usually is already reflected in the pay rate. Similarly, we can observe that any addition of more information to Pay Check would result in more inane modifications to PayRateCalculator.

Names also still do not fit: Service class is called PayRateCalculator but the service method is called CreatePaycheck.

These could be alleviated by further separation of concerns. Though it might seem too much for this small problem.

class Payment 
{
    decimal Gross;
    decimal Net;
}

class PaymentCalculator
{
    decimal _withholdingRate;
    Payment CalculatePayment(decimal hours, decimal payRate);
}

class PayCheck 
{
    string LastName;
    Payment Payment;
}

class PayCheckFactory {
    PaymentCalculator paymentCalculator;
    PayCheck CreatePayCheck(string lastName, decimal hours, decimal payRate);
}
\$\endgroup\$
  • \$\begingroup\$ thank you for that insight, almost all of this completely makes sense to me. I shouldn't perform function operations inside of properties they should be functions, is the biggest thing that I took from this. and in the beginnning is what I meant to do, but I simplified and did it wrong as you pointed out, thank you again. \$\endgroup\$ – Malachi Dec 23 '13 at 14:22

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.