current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

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

1 Answer 1

up vote 3 down vote

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 which can be refactored to

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

    if (dataRetrieveModel.ModelList.Count > 0)
    {
        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.Count > 0)
{
    dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

    if (dataRetrieveModel.ModelList.Count > 0)
    {
        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

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.