Follow up to Pay Rate Calculator

This is a Follow-up to Simple pay rate calculator

Here is what I came up with. Hopefully if I have missed something that was said in answers on the previous question, we can still get those things pointed out. I am hoping that I made the code cleaner.

Main Method

class MainClass
{
    public static void Main (string[] args)
    {
         //var payCheck = new PayRateCalculator("Hart",40,23);
        var payCheck = new PayCalculator();
        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.GrossTotal());
        Console.WriteLine ("{0}'s Net pay is ${1}", payCheck.lastName, payCheck.NetTotal());
    }
}

PayCalculator Class

```
class PayCalculator
{
private const decimal _PayRateDefault = 8.25m;
private decimal _payrate = _PayRateDefault;
private decimal _withholdingRate = 0.20m;
private decimal _hours;

public decimal hours {
get {
if (hours < 1) {
return 40;
} else {
return _hours;
}
}
set {
_hours = value;
}
}
public decimal grossPay { get;set; }
public decimal netPay { get; private set; }
public decimal payrate {
get {
return _payrate;
}
set {
if (value < 8.25){
_payrate = 8.25m;
} else {
_payrate = value;
}
}
}
public decimal WithholdingRate{ get; set; }
public string lastName { get; set; }

public decimal GrossTotal ()
{
gros

Well, you have started to use decimal instead of float and introduced a couple of named constants but pretty much every other point I stated in my answer to the other question still stands.

• Naming conventions (PascalCase for properties)

• You now have introduced a WithholdingRate property but that doesn't affect anything (the property is not used anywhere in the class)

• Although you have introduced a named constant _PayRateDefault you are still using magic numbers in various places.

• You have added some rules but they are implemented inconsistently - in hours you enforce the rule in the get while in payrate you do it in the set.

-
The way you have added the rules is simply bad - you silently coerce the values and the user of your class won't have a clue what's going on.

Assume this calculator is used to process the payroll in a company with lots of employees. Further assume that some of those are contractors which have not worked in the last week for whatever reason. Now you read in the work hours for each employee and everyone who hasn't worked at all (hours == 0) will get a full week payed - ouch.

If properties have restrictions on what range of values they should be in then enforce them in the set method by throwing an ArgumentException stating what was violated. It's better for your program to crash with an exception than to spit out bad data.

One of the few places where coercion of values makes sense is in user controls which force out of range values back into the range and then feed that back to the user (e.g. numeric up/down controls which a given min and max) but in most other cases it will do more harm than good.

Unfortunately your code has become somewhat more convoluted than the original.

StackExchange Code Review Q#38016, answer score: 7