1
\$\begingroup\$

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);
        }
\$\endgroup\$
  • \$\begingroup\$ We cannot answer your question here because it's not about improving the code but about a feature that you are trying to implement and that doesn't seem to work yet. \$\endgroup\$ – t3chb0t Feb 15 at 16:29
  • 1
    \$\begingroup\$ What do you mean that it doesn't work yet? Is the problem that I did not include the section of the controller that retrieves the data from the api to populate the elements variable? I left that out because it is not relevant to the question I am asking and it is not a public api. I can run the application with both versions of the code and display the view with the sorted data. \$\endgroup\$ – ckb888 Feb 15 at 16:34
  • 1
    \$\begingroup\$ I think I now understand what your question is so I have edited it a little bit... is this what you mean? \$\endgroup\$ – t3chb0t Feb 15 at 16:49
  • 1
    \$\begingroup\$ ok, great ;-) I have one more question.. have you removed any parts from the code that you are asking us to review? This is important on Code Review. If you want to get good and useful feedback you should not change your code before posting. Your API is async but I don't see you awaiting anything there. \$\endgroup\$ – t3chb0t Feb 15 at 16:53
  • 1
    \$\begingroup\$ The call to get the data that populates the elements object has the await. I suppose the call to populate the emp object should/could also be asynchronous? I welcome any and all feedback, but wanted to ask primarily about the read only collection. \$\endgroup\$ – ckb888 Feb 15 at 18:48
1
\$\begingroup\$

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);
}
\$\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.