How to optimize/refactor this method?

Question

Here is my method:

public JsonResult ValidateAll(string toys)
{
    Dictionary<string, object> res = new Dictionary<string, object>();
    List<string> result_check = new List<string>();

    string[] lines = toys.Split(new string[] { "\n" }, StringSplitOptions.RemoveEmptyEntries);

    foreach (string line in lines)
    {
         string toyName = line;

         string toy_tmp = toyName.Trim();
         string toy_tmp2 = HttpUtility.HtmlEncode(HttpUtility.HtmlDecode(toyName)).Replace("&#228;", "&auml;").Trim();
         string toy_tmp3 = toyName.Replace("\u00a0", " ").Trim();
         string toy_tmp4 = HttpUtility.HtmlDecode(toyName.Trim());
         string toy_tmp5 = HttpUtility.HtmlDecode(toyName.Replace("\u00a0", " ").Replace(" ", " ").Trim());

         string tableInDatabase = "";

         //Check if the toy is a doll
         tblDoll doll = (from x in db.tblDoll
                                  where (x.doll_name == toy_tmp || x.doll_name == toy_tmp2 || x.doll_name == toy_tmp3 || x.doll_name == toy_tmp4 || x.doll_name == itoy_tmp5)// || x.doll_name == toy_tmp6)
                            select x)
                            .FirstOrDefault();

         if (doll != null)
         {
             table = "doll";
             result_check.Add("1");
             continue;
         }

         if (table == "")
         {
             var car = (from x in db.tblCar
                       where (x.car_name == toy_tmp || x.car_name == toy_tmp2 || x.car_name == toy_tmp3 || x.car_name == toy_tmp4)
                       select y).FirstOrDefault();
             if (car != null)
             {
                 table = "car";
                 result_check.Add("1");
                 continue;
              }

          }

         //Do the same as for Car and Doll, do it for Plane and Boat too
    }
    res.Add("ok", result_check);
    return Json(res, JsonRequestBehavior.AllowGet);     

}

What I'm trying to do is this: the input string is a name of a toy. I have four tables for toys in my db: Car, Doll, Plane, Boat. I need to find in which table is the toy (the input string).

Answer 1

Here's a crack at a bit of code. It's probably worth mentioning though that I don't think your code could even compile or if it does your missing variables i.e. itoy_tmp5...

Either way, like Robert said I looked for some patterns that seemed to be repeating and started from there. I extracted the first part to a method and then extracted again etc etc

Here's the code anyway if it helps.

    private string[] GetPossibleNames(string toyName)
    {
        return new []
            {
                toyName,
                HttpUtility.HtmlEncode(HttpUtility.HtmlDecode(toyName)).Replace("ä", "ä"),
                toyName.Replace("\u00a0", " "),
                HttpUtility.HtmlDecode(toyName),
                HttpUtility.HtmlDecode(toyName.Replace("\u00a0", " "))
            };
    }

    private bool IsToyFor<T>(Table<T> table, string[] possibleToyNames, Func<T,string> getName)
    {
        return table.Any(p => possibleToyNames.Contains(getName(p)));
    }

    private bool HasInterestingToy(string[] possibleToyNames)
    {
        return IsToyFor(db.tblDoll, possibleToyNames, p => p.doll_name) ||
               IsToyFor(db.tblDoll, possibleToyNames, p => p.car_name);
               //Do the same as for Car and Doll, do it for Plane and Boat too
    }

    public JsonResult ValidateAll(string toys)
    {
        Dictionary<string, object> res = new Dictionary<string, object>();
        var resultCheck = new List<string>();

        string[] lines = toys.Split(new [] {"\n"}, StringSplitOptions.RemoveEmptyEntries);

        foreach (string line in lines)
        {
            string[] possibleToyNames = GetPossibleNames(line.Trim());

            if (HasInterestingToy(possibleToyNames)) resultCheck.Add("1");                
        }

        res.Add("ok", resultCheck);
        return Json(res, JsonRequestBehavior.AllowGet);
    }  

Answer 2

Well without any context I am going to go out on a limb and take a stab at this anyway.

First is if you are using a database, and have the ability to check for toy ID's instead of the toy names you won't have as many checks. it would require a little more work to pass in the string, but it would be very hard to change in the future if you get new toy names. IDs are a way of future proofing.

One trick of refactoring is to look at code that does the same thing and pulling it out into its own method. This is how I see your code working out.

public JsonResult ValidateAll(string toys)
{
    Dictionary<string, object> res = new Dictionary<string, object>();
    List<string> result_check = new List<string>();

    string[] lines = toys.Split(new string[] { "\n" }, StringSplitOptions.RemoveEmptyEntries);

    foreach (string line in lines)
    {
        var doll = CheckForToy(db.tblDoll, line);
        if(AddResult(doll, "doll", ref result_check))
            continue;

        if (table == "")
        {
            var car = CheckForToy(db.tblCar, line);
            if (AddResult(car, "car", ref result_check))
                continue;
        }
        var plane = CheckForToy(db.tblPlane, line)
        if(AddResult(plane, "plane, ref result_check))
            continue;

        var boat = CheckForToy(db.tblBoat, line)
        if(AddResult(boat, "boat, ref result_check))
            continue;
         //Do the same as for Car and Doll, do it for Plane and Boat too
    }
    res.Add("ok", result_check);
    return Json(res, JsonRequestBehavior.AllowGet);
}
private string CheckForToy(Table table, string line)
{
     string toyName = line;

     string toy_tmp = toyName.Trim();
     string toy_tmp2 = HttpUtility.HtmlEncode(HttpUtility.HtmlDecode(toyName)).Replace("&#228;", "&auml;").Trim();
     string toy_tmp3 = toyName.Replace("\u00a0", " ").Trim();
     string toy_tmp4 = HttpUtility.HtmlDecode(toyName.Trim());
     string toy_tmp5 = HttpUtility.HtmlDecode(toyName.Replace("\u00a0", " ").Replace(" ", " ").Trim());

     string tableInDatabase = "";

     //Check if the toy is a doll
     tblDoll toy = (from x in table
                              where (x.doll_name == toy_tmp || x.doll_name == toy_tmp2 || x.doll_name == toy_tmp3 || x.doll_name == toy_tmp4 || x.doll_name == itoy_tmp5)// || x.doll_name == toy_tmp6)
                        select x)
                        .FirstOrDefault();
    return toy;
}
private bool AddResult(Toy toy, string toyType, ref List<String> result)
{
    if(toy != null)
    {
        table = toyType;
        result.Add("1");
        return true;
    }
    return false;
}

There is probably even more ways to refactor my code, but without better context that is the best I can do. This is where having unit tests is super useful because you can continue to run your test every time you make a change to it to clean it up.