0
\$\begingroup\$

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).

\$\endgroup\$
  • 1
    \$\begingroup\$ What is it supposed to do, and what are you looking for? What does the input look like? A bit more context might be helpful here. \$\endgroup\$ – TeaDrivenDev Mar 3 '13 at 13:25
  • \$\begingroup\$ @GCATNM I added something, pls see. \$\endgroup\$ – Srcee Mar 3 '13 at 17:17
2
\$\begingroup\$

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("&#228;", "&auml;"),
                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);
    }  
\$\endgroup\$
  • \$\begingroup\$ What should I put instead of T in IsToyFor?? \$\endgroup\$ – Srcee Mar 4 '13 at 7:53
  • \$\begingroup\$ I was only guess at the types of tblCar etc but perhaps T might be the type of the base class for Car etc i.e. Entity ?? \$\endgroup\$ – dreza Mar 4 '13 at 8:57
1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ The CheckForToy() method unfortunately can't be that simple - the LINQ query is needed separately for each kind of toy, because they're apparently all different tables and types. \$\endgroup\$ – TeaDrivenDev Mar 3 '13 at 14:24

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.