Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Consider the following code segment written by a coworker of mine:

public ActionResult Index()
    {

        DataRetrieveModel dataRetrieveModel = new DataRetrieveModel();

        using (spc_web_trunkEntities db = new spc_web_trunkEntities())
        {
            DataRetrieveDAL dataRetrieveDAL = new DataRetrieveDAL(db);

            dataRetrieveModel.CustomerList = dataRetrieveDAL.GetCustomerList();


            dataRetrieveModel.LineList = dataRetrieveDAL.GetLineList();
            if (dataRetrieveModel.LineList.Count > 0)
            {
                dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);
            }

            if (dataRetrieveModel.LineList.Count > 0 &&
                dataRetrieveModel.ModelList.Count > 0)
            {
                dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
            }

            if (dataRetrieveModel.LineList.Count > 0 &&
                dataRetrieveModel.ModelList.Count > 0)
            {
                dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
            }
        }

        return View(dataRetrieveModel);
    }

It's a very simple controller that returns a model.

My question is given this code, how would you go about cleaning/refactoring it?

Here's my approach. First, I identified a lot of short circuiting points along with empty spaces, so I went ahead and cleaned those up first:

public ActionResult Index()
    {
        DataRetrieveModel dataRetrieveModel = new DataRetrieveModel();

        using (spc_web_trunkEntities db = new spc_web_trunkEntities())
        {
            DataRetrieveDAL dataRetrieveDAL = new DataRetrieveDAL(db);

            dataRetrieveModel.CustomerList = dataRetrieveDAL.GetCustomerList();
            dataRetrieveModel.LineList = dataRetrieveDAL.GetLineList();
            if (!dataRetrieveModel.LineList.Any())
                return View(dataRetrieveModel);

            dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

            if (!dataRetrieveModel.ModelList.Any())
                return View(dataRetrieveModel);

            dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
            dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
        }

        return View(dataRetrieveModel);
    }

I'm also wondering why multiple queries have to be made to initialize dataRetrieveModel, but for now, I only want to focus on this method.

But then all those return View(dataRetrieveModel) seems too verbose, so I thought about defining a return label and using a goto. I'm not sure how others feel about it.

I'd like to hear any suggestions on this matter.

share|improve this question

2 Answers 2

I will review the first method.

Code duplication

I see 3 times checking for dataRetrieveModel.LineList.Count > 0 and 2 times checking for dataRetrieveModel.ModelList.Count > 0 where Count > 0 should be changed to .Any() (thanks to @krillgar) and then be refactored to

if (dataRetrieveModel.LineList.Any())
{
    dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

    if (dataRetrieveModel.ModelList.Any())
    {
        dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
        dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
    }
}

This still uses 2 times dataRetrieveModel.LineList[0].Value and dataRetrieveModel.ModelList[0].Value so let's refactor again and add 2 vars holding these two values

if (dataRetrieveModel.LineList.Any())
{
    dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

    if (dataRetrieveModel.ModelList.Any())
    {
        var lineListValue = dataRetrieveModel.LineList[0].Value;
        var modelListValue = dataRetrieveModel.ModelList[0].Value;

        dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(lineListValue , modelListValue);
        dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(lineListValue , modelListValue);
    }
}

The remaining part of this method I would just keep it like it is, because it is more readable than yours and also uses only one return point at the end. Looking at your approach, I would suggest either adding {} brackets at your if statements, or at least write them in one line.

share|improve this answer
    
Also change the .Count > 0 to .Any() as that is better performing. Count iterates through the entire list, and Any will stop iterating after it finds the first item that matches the filtering. In the case of no filtering, the first item. –  krillgar Aug 7 at 13:26
    
@krillgar Thanks, answer edited –  Heslacher Aug 7 at 13:37

The entire approach is poorly written. Software development 101, not a good idea to place data access in your presentation layer.

It's also missing a service layer which is meant to be taking care of "business" related logic. Your service layer should be servicing requests from the presentation layer and returning models for display on the presentation side.

Your entity framework models should not be passing the service layer boundary either. They should be transformed in to different models which can either be utilised further by the service layer or sent to the presentation.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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