Simple worker class program

Question

I Have written this program to add workers into a list and print them. I'd like a review of this. I'm especially concerned if the method Addworker should be inside the class or in the main program.

Person:

class Person
{
    public uint NumberId;
    public string Name;
    public string LastName;
    public uint Age; // Age Should Be Beetween 18 And 40

    public uint numberId
    {
        set { this.NumberId = value; }
        get { return this.NumberId; }
    }

    public string name
    {
        set { this.Name = value; }
        get { return this.Name; }
    }

    public string lastName
    {
        set { this.LastName = value; }
        get { return this.LastName; }
    }

    public uint age
    {
        set
        {
            if (value < 18 || value > 40)
            {
                throw new ArgumentOutOfRangeException();
            }
            this.Age = value;
        }

        get { return this.Age; }
    }
}

Worker:

class Worker:Person
{
    private uint WageHour;
    private uint Total_Hours;

    public uint wageHour
    {
        set { this.WageHour = value; }
        get { return this.WageHour; }
    }

    public uint total_Hours
    {
        set { this.Total_Hours = value; }
        get { return this.Total_Hours; }
    }

    public double SalaryCalcul()
    {
        return this.wageHour * this.total_Hours;
    }

    public override string ToString()
    {
        return string.Format("Name : {0} Last Name : {1} Age : {2} WageHour : {3} Total Hours : {4} Salary : {5}",
                          this.name,this.lastName,this.age,this.wageHour,this.total_Hours,this.SalaryCalcul().ToString());

     }
}

Main program:

class Program
{
    static List<Worker> WorkersList = new List<Worker>();

    static void Main(string[] args)
    {
        do
        {
            Console.Clear();

            Console.WriteLine("1 - Add A Worker .");
            Console.WriteLine("2 - Print List .");

            int Choice = int.Parse(Console.ReadLine());

            switch (Choice)
            {
                case 1:
                    {
                        AddWorker();
                        break;
                    }

                case 2:
                    {
                        Console.Clear();

                        foreach (Worker W in WorkersList)
                        {
                            Console.WriteLine(W.ToString());
                        }

                        Console.ReadKey();

                        break;
                    }
            }
        } 
        while (true);

    }

    static void AddWorker()
    {
        Worker W = new Worker();

        Console.WriteLine("Enter ID : ");
        W.numberId = uint.Parse(Console.ReadLine());

        Console.WriteLine("Enter Name : ");
        W.name = Console.ReadLine();

        Console.WriteLine("Enter Last Name : ");
        W.lastName = Console.ReadLine();

        try
        {
            Console.WriteLine("Enter Age : ");
            W.age = uint.Parse(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.WriteLine("Age Should Be Between 18 and 40 ");
        }

        Console.WriteLine("Enter Hour Wage : ");
        W.wageHour = uint.Parse(Console.ReadLine());

        Console.WriteLine("Enter Total Hours : ");
        W.total_Hours = uint.Parse(Console.ReadLine());

        WorkersList.Add(W);
    }
}

Just wondering, but any reason SalaryCalcul() has double as a return type, while its based on two uint32s? — Caramiriel, yesterday
This is not the question I was expecting when I read the title about worker classes. Good question though. — Kris Harper, 22 hours ago

Alien Herb Nite · Accepted Answer · 2016-03-30 11:49:11Z

Properties

Currently, you're defining a public field, and then creating a property which can get and set that public field, like this:

public int SomeNumber;
public int SomeNumberProperty
{
     get { return this.SomeNumber; }
     set { this.SomeNumber = value; }
}

Firstly, the field that the property sets and gets should be private, if you choose to not use auto-properties, like this:

private int _SomeNumber;
public int SomeNumberProperty
{
    get { return this._SomeNumber; }
    set { this._SomeNumber = value; }
}

Now, while this is something that you can do, in the case of your code, there is no need to manually define the getters and setters in your properties. You can simply define an auto-property, like this:

public int SomeNumber { get; set; }

The C# compiler will automatically generate a private field in which the value for the property is stored. Do note, this value can only be accessed through the property's getters and setters.

If you're using C# 6 or later, you can also define a default value for the property, like this:

public int SomeNumber { get; set; } = 42;

Constructors

All of your classes that represent something, like a Person or a Worker are missing something rather important - a constructor. A constructor allows you to initialize the values of specific properties or fields when the object is instantiated. For example, here's a representation of a Person class using auto-properties and a constructor:

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

    public Person(int age, string name)
    {
        this.Age = age;
        this.Name = name;
    }
}

The constructor in question is this portion of code right here:

public Person(int age, string name)
{
    this.Age = age;
    this.Name = name;
}

It should be relatively self-explanatory what this chunk of code is doing. It's called when an instance of Person is created and sets the value of each property to the corresponding argument passed. An example of creating a new instance of this class might look like this:

Person person1 = new Person(42, "Bill");

By using the constructor to initialize values, it means that you don't have to initialize properties/fields like this:

Person person1 = new Person();
person1.Age = 42;
person1.Name = "Bill";

Side note: You can also use the object initializer syntax instead of constructors, but I'd recommend sticking with constructors for now.

Other nitpicks

The following method is the perfect candidate to be converted into a property:

public double SalaryCalcul()
{
    return this.wageHour * this.Total_Hours;
}

All this method does is return a value, so we can define a property like this:

public double Salary
{
    get { return this.wagehour * this.Total_Hours; }
}

If you are using a C# 6 or later, you can simply write the above like this:

public double Salary => this.wagehour * this.Total_Hours;

Simply attempting to convert user input to an integer isn't the best idea. What if the user enters a string, like "Hello"? To ensure that no errors are caused due to faulty user data, you should set up something like this:

int convertedUserInput;
string userInput = Console.ReadLine();

if(Int32.TryParse(userInput, out convertedUserInput))
{
    // the user entered a proper integer
}
else
{
    // the user did not enter a proper integer
}

As a side note, properties should also be named in PascalCase, along with fields.

EBrown · Answer 2 · 2016-03-31 07:45:19Z

A couple of quick notes:

Generally speaking, in C# we try to use PascalCase (first letter of each word is uppercase) for public and protected members (properties, fields (never have public fields except when absolutely necessary), events, methods, classes, interfaces) and camelCase (first letter of each word is uppercase, excepting the first word) for private members, and locals (variables that are only contained within a method/property). We don't use snake_case or SHOUTY_SNAKE_CASE anymore, though I won't lie, I still use SHOUTY_SNAKE_CASE for constants.

If you want to help keep private members a little more distinguished, we also commonly use the underscore (_) as a prefix to private members. I do this almost always, as it helps me distinguish between them a bit better from method parameters and such (which do not get the prefix). This also means we can avoid using the mParameterName naming convention which was popular at one point.

Please, please, please always explicitly include access modifiers. Right now, you may not realize it, but Person and Worker are both internal. This means they cannot be accessed from outside the assembly they are written in, which isn't very useful if you have to share your implementations with another person/developer. If you want to keep them internal, then mark them as such so you don't forget the scope of them when you go to debug the issue of why you cannot access them from other stuff. (Trust me, I've been there and done that, and it's not fun.)

Also, we try not to abbreviate too much in C#. Something like SalaryCalcul isn't really necessary, and instead should better reflect what the method is doing: GetSalary is a much better name, but even if you stick with the original intent, SalaryCalculate is also generally expected.

Let's talk about this for a moment:

public uint age
{
    set
    {
        if (value < 18 || value > 40)
        {
            throw new ArgumentOutOfRangeException();
        }
        this.Age = value;
    }

    get { return this.Age; }
}

The numbers 18 and 40 are now known as "magic numbers" in your programme. The problem with this is that if you need to make that same restriction elsewhere, you have to hardcode 18 and 40. Instead, you should consider making two properties, MaxAge and MinAge, which you can default to 40 and 18 respectively, and pass other values to if necessary.

Just as well, you should pass two strings to your ArgumentOutOfRangeException: the first is the name of the parameter (value in this case), and the second is the specific message. In this instance, if I pass 41 to an age, I have no idea why it's out of range. I am not given any information as to what the acceptable range is, so now I have to consult the documentation (you do have documentation, right?) to find it. This is where your MaxAge and MinAge come in, you can pass a message $"Valid ages are {MinAge} to {MaxAge}.".

This brings up another quick point about a nice C#6.0 feature: string interpolation. Much like PHP has had for some time, C#6.0 has added the ability to pass variables directly into a string. Simply prefix the string literal with a dollar-sign ($), and then wrap the variable in braces ({}). The .ToString() method will be called on the variable, and the result of that method will be replaced into the string.

You don't handle input errors well (or at all, but that's alright, you are a beginner) so I'm going to take this moment to shamelessly promote a class I wrote which can help you with a great deal of ease here.

You should consider grabbing all the files in this folder on GitHub and using the ConsolePrompt to handle your input. It's extremely simple to use, just create an instance of ConsolePrompt and call .Prompt on it. You'll also need to grab the ILogger and at the very least the EmptyLogger from this other folder on GitHub. (Or remove all references to ILogger, but that's much more difficult.) I've linked to both of these as of the most current version of the writing, just in case I make any changes after this answer is published.

var logger = new EmptyLogger();
var consolePrompter = new ConsolePrompt(logger);
var Choice = consolePrompter.Prompt<int>("Enter the desired option", PromptOptions.Required);

You can reuse (and pass) the consolePrompter object to other classes/methods, so you should only ever need to create one of them for things to work properly.

You can also do much more with it, like passing a validation method to ensure that the user only enters the numbers you expect and such, so I leave that for you to discover.

Another quick note (that I forgot when I wrote this at 0200 in the morning): you can get away with using var in many cases, where the type is already explicit. (It just allows you to shorten the line of code a little bit.) For example:

Worker W = new Worker();

Can be rewritten as:

var W = new Worker();

It's a pretty trivial issue, but I didn't see any use of it in your programme so I haven't the foggiest if you knew of it or not.

All-in-all this is a very good start for a beginner to C# programming, much better than my first entanglement with the language. I'm quite interested to see what else you manage to accomplish while experimenting in the language. :)

Do note: any C#6.0 features require Visual Studio 2015.

Malachi · Answer 3 · 2016-03-29 19:59:50Z

You were right to have the AddWorker method outside of the Worker class, but you should be returning a Worker object from that method rather than adding to the list from within. adding to the list from within tightly couples that method to that list that may or may not be there when you call the method.

Your AddWorker doesn't change much, but I would rename it.

public static Worker CreateNewWorker()
{
    Worker W = new Worker();

    Console.WriteLine("Enter ID : ");
    W.numberId = uint.Parse(Console.ReadLine());

    Console.WriteLine("Enter Name : ");
    W.name = Console.ReadLine();

    Console.WriteLine("Enter Last Name : ");
    W.lastName = Console.ReadLine();

    try
    {
        Console.WriteLine("Enter Age : ");
        W.age = uint.Parse(Console.ReadLine());
    }
    catch (Exception)
    {
        Console.WriteLine("Age Should Be Between 18 and 40 ");
    }

    Console.WriteLine("Enter Hour Wage : ");
    W.wageHour = uint.Parse(Console.ReadLine());

    Console.WriteLine("Enter Total Hours : ");
    W.total_Hours = uint.Parse(Console.ReadLine());

    return w;
}

What we are really doing here is creating a new Worker and returning it to whoever called it.

I personally don't like using a do while unless I absolutely have to, so I changed it to a plain while statement, but you should have an option to allow the user to dump out of the loop.

you should also give your switch a default case so that you can let the user know that they didn't input correctly.

I also brought the list variable inside of the Main method and gave it a more clear name, workers should be just fine for a list of workers, I also gave the name camalCasing which is appropriate for a variable.

so we change our code in the Main method like this

static void Main(string[] args)
{
    var workers = new List<Worker>();

    while (true)
    {
        Console.Clear();

        Console.WriteLine("1 - Add A Worker .");
        Console.WriteLine("2 - Print List .");

        int Choice = int.Parse(Console.ReadLine());

        switch (Choice)
        {
            case 1:
            {
                workers.add(CreateNewWorker());
                break;
            }
            case 2:
            {
                Console.Clear();

                foreach (Worker W in workers)
                {
                    Console.WriteLine(W.ToString());
                }
                Console.ReadKey();
                break;
            }
            default:
            {
                Console.WriteLine("invalid entry");
            }
        }
    }
}

Something to think about in your CreateNewWorker method is the Try Catch

    try
    {
        Console.WriteLine("Enter Age : ");
        W.age = uint.Parse(Console.ReadLine());
    }
    catch (Exception)
    {
        Console.WriteLine("Age Should Be Between 18 and 40 ");
    }

once you catch the bad input, you tell the user they have input bad input and then continue without ever setting the age for this Worker you may want another loop here that is exited when the age is set to a good value.

I don't like the way that you are using an exception to handle the user input, you haven't even checked to make sure that you have caught the exception that you were anticipating, you automatically assume that the user typed a number outside of the range to receive an error. In a larger application there could have been a different error than what you are expecting and you wouldn't know what it was, you would be testing by inputting 20 for the age and still getting the error message.

asked	yesterday
viewed	948 times
active	today

current community

your communities

more stack exchange communities

Simple worker class program

3 Answers 3

Properties

Constructors

Other nitpicks

A couple of quick notes:

Your Answer

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

Hot Network Questions

current community

your communities

more stack exchange communities

Simple worker class program

3 Answers 3

Properties

Constructors

Other nitpicks

Did you find this question interesting? Try our newsletter

A couple of quick notes:

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions