7
\$\begingroup\$

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 ()
    {
        grossPay = hours * payrate;
        return grossPay;
    }

    public decimal NetTotal ()
    {
        grossPay = GrossTotal();
        netPay = grossPay - (grossPay * _withholdingRate);
        return netPay;
    }

    public PayCalculator (string lastName, decimal hours, decimal payRate)
    {
        this.hours = hours;
        this.payrate = payrate;
        this.lastName = lastName;
    }

    public PayCalculator ()
    {
    }
}

I am going to put the tag on this again because I know that my coding skills are still not very good.

This will be the last review for this little bit of code. I am going to see what else catches my eye in this book while I am working through it.

\$\endgroup\$
7
\$\begingroup\$

There are several things to address here:

Naming Conventions: only fields, method parameters, and local variables should be camelCase, everything else should be PascalCase. This doesn't make your programs run better, but it does make you a more likable programmer in the workplace. Being able to produce code that is consistent with your co-workers will help you stay employeed. No one likes a rogue programmer.

Validate all external input: in your application the external input is coming from the info provide by the user of your program. It could be a text box in a windows application or data provided to a web service that you have written. In all cases you need to validate the input before processing so that you can provide the user with an appropriate message. In your case there are places where valid values cannot be less than zero. Check for that and alert the user of their mistake. Here is an example that verifies the pay rate and ensures that it is at least the minimum pay rate.

decimal GetRate(string lastName)
{
    decimal rate = -1m;
    string tmp = string.Empty;
    do
    {
        Console.Write("What is employee {0}'s hourly wage? >>", lastName);
        tmp = Console.ReadLine();
        if (!decimal.TryParse(tmp, out rate) || rate < Paycheck.MinimumHourlyRate)
        {
            Console.WriteLine("hourly wages must be a valid number greater than {0:#######.00}", Paycheck.MinimumHourlyRate);
            rate = -1m; // set the rate to an out of bounds value because the TryParse call may have returned false
        }
    } while (rate < Paycheck.MinimumHourlyRate);
    return rate;
}

Do not let objects be created in an invalid state: This is not always possible to achieve, but you have total control over this code. Your PayCalculator class has computed values for both gross and net pay. Neither of these values make sense with out an appropriate pay rate and number of hours. Therefore all constructors should take, at a minimum, these two values. This ensures that your PayCalculator does not get created in an invalid state.

Do not let objects become invalid: You should be checking that values are in range when being assigned. This is different than the validation mentioned earlier. For example negative hours make no sense in this program. You should throw an appropriate exception if someone tries to set the number of hours worked to a negative value:

decimal _numberOfHours;
public decimal NumberOfHours
{
    get
    {
        return _numberOfHours;
    }
    set
    {
        if (value < 0)
            throw new ArgumentOutOfRangeException("NumberOfHours", value, "value cannot be less than zero");
    }
}

If you perform proper validation as discussed earlier, this exception should never be thrown. However, the whole point of exceptions is for exceptional situations that are not supposed to occur.

Principle of least astonishment: You have places in your code that assume a default value when the supplied value is out of range. For example, in the property setter of payrate:

if (value < 8.25)
  _payrate = 8.25m;

A user of your code that tries to assign 1.0m to the payrate property is not expecting that value to magically change to 8.25m. If you refer back to the example for the NumberOfHours property you will see that I am not defaulting a negative value to zero. I am throwing an exception. It is the user's responsibility to provide your PayCalculator class with valid values. In order to facilitate this you should be documenting your code. The NumberOfHours example property should clearly document that it will throw an exception if someone tries to assign a negative value to that property.

Conclusion The items explained above should be carried beyond this program and taken into consideration for all programs that you write. They will make you a better programmer in the long run if you adopt them now.

There are also a few other things in your code that are incorrect. Those are simply bugs in your code

  • WithholdingRate does not use its backing field
  • You are using Convert.ToInt32 for a decimal value
  • using Convert.ToInt32 or the correct Convert.ToDecimal function can throw exceptions if text is entered. You should code for that. I recommend using decimal.TryParse instead.
  • the payrate property setter checks value < 8.25, 8.25 is going to be a double. double values can lose precision. It won't lose precision in the case of 8.25 but you should be doing 8.25m so that this problem does not arise
\$\endgroup\$
  • 1
    \$\begingroup\$ Very nice organization, and some great points. +1 \$\endgroup\$ – syb0rg Jan 3 '14 at 3:47
7
\$\begingroup\$

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.

  1. Naming conventions (PascalCase for properties)
  2. You now have introduced a WithholdingRate property but that doesn't affect anything (the property is not used anywhere in the class)
  3. Although you have introduced a named constant _PayRateDefault you are still using magic numbers in various places.
  4. 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.
  5. 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.

\$\endgroup\$
  • \$\begingroup\$ the WithholdingRate is used to figure out the net total. I had a reason for putting rules on Setting the payrate but I am thinking of going a different way with it, in that I would produce a message for trying to set the rate less than a minimum, which would be kind of like the exceptions that you are talking about. \$\endgroup\$ – Malachi Dec 24 '13 at 23:43
  • 1
    \$\begingroup\$ @Malachi: The property WithholdingRate is not used at all. You use the private field _withholdingRate but that has no relationship with the public property. \$\endgroup\$ – ChrisWue Dec 25 '13 at 0:45
  • \$\begingroup\$ I am going to have to look into the Private field/public property thing, because I might be operating under a false impression. \$\endgroup\$ – Malachi Dec 26 '13 at 2:06
5
\$\begingroup\$

Some enhancements I'd make is making employee a separate class with name, payrate, and a list of another class,workday that holds days and hours worked in the current pay period. If you make the PayRate class static and have only methods that take an object of class employee you simplify the class quite a bit.

\$\endgroup\$
  • \$\begingroup\$ I was thinking about something similar to what I think you are saying, but I was thinking that should be more of a SQL/database stuff, I figure that is going to be the next step. \$\endgroup\$ – Malachi Dec 24 '13 at 7:23
  • 2
    \$\begingroup\$ Having your code already like that will help when you implement storing the data. \$\endgroup\$ – tinstaafl Dec 24 '13 at 7:24
3
\$\begingroup\$

Not to detract from all the above useful posts but I don't think your code is going to work. Recommend you fix it (replace hours with _hours in the line indicated) so that it conforms to the "code must work!" guidelines in CodeReview

public decimal hours { 
    get {
        if (hours < 1) { // <------------ Recursion?
            return 40;
        } else {
            return _hours;
        }
\$\endgroup\$
  • \$\begingroup\$ it does work, I tested it before I posted it. \$\endgroup\$ – Malachi Jan 2 '14 at 23:01
  • \$\begingroup\$ Please explain how the recursive call to hours is going to make your code work? \$\endgroup\$ – shivsky Jan 2 '14 at 23:11
  • \$\begingroup\$ it functions the way that I anticipated it would function, if the hours variable was less than 1 it would return 40 hours. otherwise it will return the value held in the hours variable. as pointed out by @ChrisWue it is a design flaw and not a functionality flaw. it will function but the design is bad, so it should be changed, and will be, but the code still works. \$\endgroup\$ – Malachi Jan 3 '14 at 0:13
  • 1
    \$\begingroup\$ PLEASE (I'm going to resort to begging now) pay attention to my comment in the code-snippet copied from your code. Your property is recursively calling itself. Stack overflow!!! You've evidently made changes to this posted code after you tested it as a simple copy-paste into my VS2012 IDE reveals this "code will fail" problem rather loudly. This might help you understand what I'm getting at. stackoverflow.com/questions/12044505/… \$\endgroup\$ – shivsky Jan 3 '14 at 1:51
  • 1
    \$\begingroup\$ I see what you are saying, it should be _hours instead of hours \$\endgroup\$ – Malachi Jan 3 '14 at 3:53
1
\$\begingroup\$

I made a small mistake in my Code that causes an issue when trying to edge test

this code

public decimal hours { 
    get {
        if (hours < 1) {
            return 40;
        } else {
            return _hours;
        }
    }
    set {
        _hours = value;
    }
}

was changed to this

public decimal hours { 
    get {
        if (_hours < 0) {
            return 40;
        } else {
            return _hours;
        }
    }
    set {
        _hours = value;
    }
}

so that it doesn't crash when entering 0, and if something less than 0 is entered it automatically gives a default value.

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