Sorting and search List with Icomparer

I would like to ask for code review for my project related to sorting and search C# lists with IComparer on objects. I expect some design tips and how to make code more effective.

Job class:

class Job
{
    public string Title { get; set; }
    public int Salary { get; set; }
    public string Category { get; set; }

    public void AddJob()
    {
        Console.Write("Title: ");
        Title = Console.ReadLine();

        Console.Write("Salary: ");
        int sal;
        while (!int.TryParse(Console.ReadLine(), out sal))
        {
            Console.WriteLine("Salary must be an integer. Please try again.");
        }
        Salary = sal;

        Console.Write("Category: ");
        Category = Console.ReadLine();
    }

    public void Print()
    {
        Console.WriteLine("Title: {0}", Title);
        Console.WriteLine("Salary: {0}", Salary);
        Console.WriteLine("Category: {0}", Category);
    }

}

JobList class:

```
class JobList
{
private List jobList = new List();

public void AddToList(Job job)
{
jobList.Add(job);
}

public void PrintList()
{
int index = 0;
foreach(Job j in jobList)
{
Console.WriteLine("**");
Console.WriteLine("Index: {0}", index);
j.Print();
index++;
}
}

public void SortTitleAscending()
{
NameComparerAscending nameAscending = new NameComparerAscending();
jobList.Sort(nameAscending);
}

public void SortTitleDescending()
{
NameComparerDescending nameDescending = new NameComparerDescending();
jobList.Sort(nameDescending);
}

public void SortSalaryAscending()
{
SalaryComparerAscending salaryAscending = new SalaryComparerAscending();
jobList.Sort(salaryAscending);
}

public void SortSalaryDescending()
{
SalaryComparerDescending salaryDescending = new SalaryComparerDescending();

OrderBy over sort

You can save yourself a lot of code here by using LINQ's OrderBy and OrderByDescending. These allow you to use a lambda to pull out a part of the object to order by, and the default IComparer for the type returned by the lambda is used. So for example:

public void SortTitleAscending()
{
    jobList.OrderBy(job => job.Title);
}

Allowing you to completely remove your IComparers (they're also used in BinarySearch, but I'll go into that later)

Creating Jobs

There are a number of issues with how you create jobs:

• The method is called AddJob, but if you do myJob.AddJob(), you're not actually adding a job to myJob, you're initializing myJob.

• Because that method is not static, you first have to create a job object, then call a method on it to actually correctly initialize it. Before then, it looks like you have a job, but really it's in an invalid state

• There's in fact no reason to have a special AddJob method at all- creating an instance of a class is precisely what a constructor is for.

• Because the parameters on Job are public, you can just completely ignore that method and set the parameters however and whenever you want. To me it seems more appropriate for those properties to be immutable - i.e. once you've constructed a job, you can't just arbitrarily change it from wherever you want.

• The AddJob method directly interacts with the console. This is a violation of what's called the Single Responsibility Principle. It's the Job's responsibility to be a Job, not to be a JobWhichGetsInputFromTheUserInAParticularWay. What if you later want to import jobs from a database or a text file, or add a GUI, or have some logic for creating multiple variations on an existing job? The method which creates a job should take parameters. If you want to get the value of these parameters from the console, you can do that from outside the class and pass them in.

• And finally, this isn't related to the creation but follows on from the above: The Print method has exactly the same issue as AddJob. If you want the Job to be able to report itself in the form of a string, it should return a string. It shouldn't know how that string is going to be used (in this case displayed in the console).

So, let's put that all together and come up with an alternative version of the Job class:

class Job
{
    public string Title { get; private set; }
    public int Salary { get; private set; }
    public string Category { get; private set; }

    public Job(string title, int salary, string category)
    {
        Title = title;
        Salary = salary;
        Category = category;
    }

    public override string ToString()
    {
        return String.Format("Title : {1}{0}Salary: {2}{0},Category: {3}",
            Environment.NewLine, Title, Salary, Category);
    }
}

Going through the changes:

• AddJob is removed, and replaced with a constructor

• The constructor takes parameters, rather than reading from the console

• Print has been replaced with an override of ToString, since this is an in-built method for handling how an object should be read as a string. Like the constructor, it no longer uses the console directly, just returning a string

• The properties' setters have been set to private, so that once a Job is created, it can't be changed from outside the class. If you do want one of those properties to be able to be set externally, you can remove the private, but make sure this is actually a good idea.

For an example of how you might use some validation with this class, consider adding the rule that the salary must be non-negative. You can do this by altering the property like so:

private int _salary;

public int Salary
{
    get { return _salary; }
    set
    {
        if(value < 0)
            throw new ArgumentOutOfRangeException("value", "Salary must be non-negative");
        _salary = value;
    }
}

This may or may not be something you actually want to do, but it's a good example of how a class like Job can add value through doing validation. And speaking of value:

What is the purpose of JobList?

Wrapping a collection in a class which serves as a gateway to making calls on that collection is relatively common, and often a good idea. But make sure there's good reason for it first.

The name JobList is the first indication that this may not be a case where you actually want to do this. Usually if a collection should have its own class, it's because that collection is its own conceptual entity. For example, a List might be wrapped in a Hand or Deck, or a List might be wrapped in a Team. These can all perform functions related to what they actually are which a simple List cannot. For example:

• Many card games have a limit to how many cards you can hold in your hand. So a Hand can have an Add method which first validates that the number of cards in the wrapped list is not equal to the hand capacit

StackExchange Code Review Q#52705, answer score: 7