Making EmployeeHistory ViewModel immutable as code analysis suggested

Question

I want to display on a view the EmployeeHistory view model sorted by elements.assignEffDateNext and I want to instantiate and populate the view model from the controller. The code analysis is saying that the IList<IDMSElementDate> EmployeePositionsOverTime should be read only. I kind of understand why.

This is what I started with.

public class EmployeeHistory
{
    public int EmployeeID { get; set; }
    public string EmployeeName { get; set; }
    public IList<IDMSElementData> EmployeePositionsOverTime { get; set; }

}

public async Task<ActionResult> EmployeeHistory(int? employeeID)
    {...//this part retrieves the elements from an api...

        Employee emp = db.Employees.Find(employeeID);
        string employeeName = emp.EmployeeFullName;

        var sortedList = elements.Elements.OrderByDescending(e => e.assignEffDateNext).ToList();                                                                                         

        EmployeeHistory empHist = new EmployeeHistory
        {
            EmployeeID = (int)employeeID,
            EmployeeName = employeeName,
            EmployeePositionsOverTime = sortedList,
        };


        return View("EmployeeHistory", empHist);
    }

This is what I have changed it to. I added a constructor and changed the properties to be read-only so the type is immutable now and the analyzer is no longer giving me a warning. I was wondering whether this is better now and whether there is anything else about it that I should change?

 public class EmployeeHistory
{

    public EmployeeHistory(int employeeID, string employeeName, IList<IDMSElementData> employeePositionsOverTime)
    {
        EmployeeID = employeeID;
        EmployeeName = employeeName;
        EmployeePositionsOverTime = employeePositionsOverTime;
    }

    public int EmployeeID { get; }
    public string EmployeeName { get;  }
    public IList<IDMSElementData> EmployeePositionsOverTime { get;  }

}




public async Task<ActionResult> EmployeeHistory(int? employeeID)
        {...//this part retrieves the elements from an api...

            Employee emp = db.Employees.Find(employeeID);
            string employeeName = emp.EmployeeFullName;

            var sortedList = elements.Elements.OrderByDescending(e => e.assignEffDateNext).ToList();

            EmployeeHistory empHist = new EmployeeHistory((int)employeeID,employeeName,sortedList);


            return View("EmployeeHistory", empHist);
        }

Answer 1

Nulls: What happens when someone asks for an EmployeeHistory without passing in an employee ID? It's hard for me to believe that the API for retrieving history will behave well, must less the call to db.Employees.Find(), and certainly not the cast (int)employeeID.

I would recommend either immediately checking this and bailing out (with an ArgumentNullException or an empty view, or whatever you think is best), if you aren't already doing so. Or, you could the signature to EmployeeHistory(int employeeID) - since without an employee ID there is very little you can do.

---

Names: You have an EmployeeHistory object, with three properties all named Employee-something. This is a bit redundant. Perhaps just ID, Name, and PositionsOverTime? Or PositionsHeld?

Although that would imply that that EmployeeHistory.ID is the ID of the history... But that raises the question to me, why copy these properties from the Employee object at all? Why not just hold a reference to the actual Employee?

In fact, perhaps it would make more sense to add the history of positions held to the Employee object. That does make more sense: "This employee has this history", rather than the other way around.

As a side note, I'll grant that abbreviations like emp, e, and empHist are easy enough to parse in this case. However, my preference would still be for complete words when possible. Remember that code is read far more often than it is written, so if a few extra keystrokes now save a few seconds of parsing later, that's a net win.

---

Encapsulation: You may have satisfied the requirement of a read-only collection in the eyes of your code analyzer, but you have not done so in fact. While a consumer of this class would not have the ability to swap out the entire list for one of their own, the IList<T> interface is permissive enough that they may as well. Here's a demo.

If you want to make this history truly read-only, you'll need to avoid handing out a direct reference to the list. List.AsReadonly() is good for this.

Another thing you may want to encapsulate is the order of the list. You have decided* that the history should be sorted with the most recent assignEffDateNext first. If you want to guarantee that histories are always ordered this way, the logic to do so belongs in the history's constructor.

*As a side note, I personally would not sort this list on the back end at all. Suppose a user wants to view the list in chronological order? Or sorted by position title? Javascript is perfectly capable of sorting arrays of the size you're likely to have. This gives your users flexibility while keeping (an admittedly very small) load off the server.

---

Robustness: "Be conservative in what you send, be liberal in what you accept." It's generally nice when functions only ask for what they need. As it stands, an EmployeeHistory cannot be constructed with a Collection of positions - even though just a simple IEnumerable ought to be sufficient. Save yourself future calls to .ToList(), and open up this constructor to be more permissive.

This same principle, in my mind, suggests that you should be as specific as possible about what you're returning. Making positions available as an IReadOnlyList is good, because it allows your users to access positions by index. For the same reason, I would also be more specific about what you're returning from the view. It is an ActionResult, yes... But more specifically it's a ViewResult. This may not have a big impact on your front end, but it could help with testability or refactoring down the line.

---

Here's how this code might look when you're done:

public class EmployeeHistory
{
    public Employee Employee { get; }
    public IReadonlyList<IDMSElementData> PositionsHeld { get; }

    public EmployeeHistory(
        Employee employee,
        IEnumerable<IDMSElementData> positions)
    {
        Employee = employee;

        PositionsHeld = positions
            .OrderByDescending(position => position.assignEffDateNext)
            .ToList()
            .AsReadOnly();
    }
}

public async Task<ViewResult> EmployeeHistory(int employeeID)
{
    var positions = GetPositionsFromSomeAPICall(employeeID);

    var employee = db.Employees.Find(employeeID);

    var history = new EmployeeHistory(employee, positions);

    return View("EmployeeHistory", history);
}