Sorting and search List with Icomparer

Question

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<Job> jobList = new List<Job>();

    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();
        jobList.Sort(salaryDescending);
    }

    public void SortCategoryAscending()
    {
        CategoryComparerAscending categoryAscending = new CategoryComparerAscending();
        jobList.Sort(categoryAscending);
    }

    public void SortCategoryDescending()
    {
        CategoryComparerDescending categoryDescending = new CategoryComparerDescending();
        jobList.Sort(categoryDescending);
    }

    public int SearchByName(string searchName)
    {
        NameComparerAscending nameComparer = new NameComparerAscending();
        Job job = new Job();
        job.Title = searchName;
        return jobList.BinarySearch(job, nameComparer);
    }

    public int SearchByCategory(string category)
    {
        CategoryComparerAscending categoryComparer = new CategoryComparerAscending();
        Job j = new Job();
        j.Category = category;
        return jobList.BinarySearch(j, categoryComparer);
    }
}

CategoryComparerAscending class:

class CategoryComparerAscending : IComparer<Job>
{
    public int Compare(Job x, Job y)
    {
        return String.Compare(x.Category, y.Category);
    }
}

CategoryComparerDescending class:

class CategoryComparerDescending : IComparer<Job>
    {
        public int Compare(Job x, Job y)
        {
            return String.Compare(y.Category, x.Category);
        }
    }

NameComparerAscending class:

class NameComparerAscending : IComparer<Job>
{
    public int Compare(Job x, Job y)
    {
        if (x == y) return 0;
        if (x == null) return -1;
        if (y == null) return 1;
        return String.Compare(x.Title, y.Title);
    }
}

NameComparerDescending class:

class NameComparerDescending : IComparer<Job>
{
    public int Compare(Job x, Job y)
    {
        return String.Compare(y.Title, x.Title);
    }
}

SalaryComparerAscending class:

class SalaryComparerAscending : IComparer<Job>
{
    public int Compare(Job x, Job y)
    {
        if(x.Salary > y.Salary)
        {
            return 1;
        }

        if(x.Salary < y.Salary)
        {
            return -1;
        }
        else
        {
            return 0;
        }
    }
}

SalaryComparerDescending class:

class SalaryComparerDescending : IComparer<Job>
{
    public int Compare(Job x, Job y)
    {
        if(x.Salary < y.Salary)
        {
            return 1;
        }

        if(x.Salary > y.Salary)
        {
            return -1;
        }
        else
        {
            return 0;
        }
    }
}

Some questions:

• Should I check for null value in Icomparer (I saw it in some online resources and MSDN)?
• Compare method, as parameter should I use object and declare type in method or is it ok that way using Job x?
• For using BinarySearch on list, is there better way to pass searched string orint to object value? (Last two methods in JobList class)

Answer 1

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<PlayingCard> might be wrapped in a Hand or Deck, or a List<Player> 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 capacity before adding a card.
• Many card games also have a limit to how many times the same card can appear in a deck, which like the above can be done by a Deck as validation at the points where you add one or more cards
• You want to be able to replace players in a team with other players, meaning you need to be able to add and remove them. But in many sports and games, there must be exactly the right number of players on a team. So a Team would not expose an Add and Remove method, but instead a Replace method, which would internally call Add and Remove on its wrapped list.

So those are just examples. It's not a hard and fast rule that you can't have a class called something like JobList that doesn't have a useful, sensible function in wrapping a collection, but it's a good starting point.

So in this case, let's go through what the JobList does and see if it adds value:

• Prevents arbitrary methods being called on the job list. Often this is a good thing (usually you don't want any old code being able to Clear a publically exposed list just because it feels like it), but in this case it's not obvious that that's the case. Because the name JobList doesn't tell you anything more than that it's a list of jobs- exactly the same information as you'd get from List<Job>- it's not clear that there's any thing you should be able to do to a List<Job> but not a JobList. In the Hand, Deck and Team examples, it's obvious what limited set of methods you want to expose publically, but in this case it isn't.
• Prevents external code from reading what's in the list. This is probably a bad thing- while you don't want arbitrary code modifying a collection, there's often no good reason to prevent reading it. Of course a class can have a private collection that only it cares about because it's about some implementation detail of its own (a cache used by one of its private methods, for example), and in that case, keeping the collection private is a good thing. But this is a JobList, its only apparent function is to wrap a collection of Jobs. And what good is that if once a Job has been added to the collection, you can't get it back?
• Provides a way of printing the entire list of jobs. Like in Job, this shouldn't write to the console, but if it was modified to simply return a string, this would be useful, and it's a relatively appropriate place to put it, so it would provide a small amount of value.
• Provides Sort methods. Because of the simplicity of sorting with OrderBy and OrderByDescending, there's no longer any need for this.
• Allows you to search by name and category. At the moment, this is pointless. It returns the index of the item, but since you can't access the list from outside the code, you can't do anything useful with the index. If they returned the actual item, they could potentially be useful.

So if we removed this class, how would printing and searching be handled? Or is having them where they are valuable enough to justify the class's existence?

Removing JobList

• Printing: this could be handled closer to where you're actually doing the printing. I suspect this is not something that would pop up all over the place in an application. Probably you're only writing a list of jobs to the console (or whatever output) in a human-readable form in one place. So if it was in your main Program class, for example, you could just move JobList's Print to a private static method on Program which takes the JobList as a parameter.

• Searching: BinarySearch is good if you need a search which performs very fast but... you probably don't. You're unlikely to have a huge number of jobs, and your design should never be based on shaving microseconds off some functionality unless you know that it's going to be performed frequently enough for that to be important. Plus, since you have to build a Job every time you want to do a search, in some cases it might actually be slower than the alternative. Don't prematurely optimize. With that in mind, the searches can be replaced with simple LINQ statements. Your options are:

• Where if you want all jobs matching the search criteria

• First if you want the first job matching the search criteria, and regard there being zero jobs matching the search as an error/exceptional.
• FirstOrDefault if you want the first job matching the search criteria, and do not regard there being zero jobs matching the search as an error/exceptional.
• Single if you want the only job matching the search criteria, and regard there being zero jobs or multiple jobs matching the search as an error/exceptional.
• SingleOrDefault if you want the only job matching the search criteria, and do not regard multiple jobs matching the search criteria as an error/exceptional, but not zero jobs.

In this example, I assume multiple jobs can match a category, so Where would probably be the best option if you want all of them. If you only want any one of them, then FirstOrDefault would probably fit. I'd also guess only one job can have a particular Title, so you'd probably want SingleOrDefault. In a case where you absolutely expect there to be a job matching your search criteria, then you'd want First or Single instead of FirstOrDefault or SingleOrDefault respectively.

Whichever of those LINQ methods you use, you want to pass the same thing to them: a lambda which returns true for an item matching the search criteria, and false otherwise. So for example:

string developerTitle = "Software Developer";
Job developerJob = jobs.SingleOrDefault(j => j.Title == developerTitle);

With this method, there's now no longer any need for the Search methods, and so now there's no longer any method that needs to be on JobList and so you can remove the class, just using a List<Job> instead.

Renovating JobList

The above would be my suggestion, but it's based on certain assumptions. What if Print is called in multiple places? What if BinarySearch really does perform better and that performance is critical? What if there's some additional logic or validation which that class could usefully handle? In those cases, you may want to keep JobList. So instead of deleting it, here's how you could improve it:

• Consider whether there's a better name. Like Team over PlayerList or Hand over PlayingCardList, is there some real-world concept that this list of jobs corresponds to? Renaming it will not only make your code clearer, it'll immediately become much more obvious what it is and isn't appropriate for this class to handle.
• Expose a read-only version of the list. You can do so like this: public IEnumerable<Job> Jobs { get { return jobList; } } This will return a collection that any class can iterate over, query, or whatever, but not modify. (Note that in theory another class could cast this to a List<Job> and modify it, but at that point you're talking about such antisocial behaviour that there really isn't any way to fully protect against it)
• Remove your sort methods, and the IComparers not used by the search methods. In this case, you'll have to keep the IComparers which are used by the search methods.
• Have the search methods return the actual Job they find, rather than its index.
• Convert PrintList to a ToString which returns a string rather than writing to the console, just like in Job

Answer 2

One advantage of using IComparer interface is the ability to use one routine for ascending or descending comparisons. This mixed with Reflection allows you to use one routine to compare any of the 3 properties. If you plan on more different types of properties this might not be too practical but should work for the class you've shown:

class JobPropertyComparer : IComparer<Job>
{
    private string _property = "";
    private int order = 1;
    public JobPropertyComparer(string property, bool asc)
    {
        if(!asc)
            order = -1;
        _property = property;
    }

    int IComparer<Job>.Compare(Job x, Job y)
    {
        System.Reflection.PropertyInfo propertyxtocompareby = x.GetType().GetProperty(_property);
        System.Reflection.PropertyInfo propertyytocompareby = y.GetType().GetProperty(_property);
        var valuea = propertyxtocompareby.GetValue(x, null);
        var valueb = propertyytocompareby.GetValue(y, null);
        if(valuea.GetType() == typeof(string))
        {
            return (((string)valuea).CompareTo((string)valueb)) * order;
        }
        else
            if(valuea.GetType() == typeof(int))
                return (((int)valuea).CompareTo((int)valueb)) * order;
        return 0;
    }
}

Now sorting by any property could look like this:

    public void Sort(string property, bool asc)
    {
        jobList.Sort(new JobPropertyComparer(property, asc));
    }

Searching by any property and returning the object could look something like this:

    public Job Search(string property, Job value )
    {
        return jobList[jobList.BinarySearch(value, new JobPropertyComparer(property, true))];
    }