1
\$\begingroup\$
    if(selectedGroup == 0 && model.selectedParameterItem != null)
    {
        foreach (var i in model.selectedParameterItem)
        {
            parameters.Add(db.ParametersMethods.Where(f => f.ID == i).FirstOrDefault());
        }
        methods.AddRange(methodlogicclass.GetMethodFromParameters(parameters));
        ViewBag.methods = convertMethod(method);

        return View(methodModel);
    }

    else if (selectedGroup !=0 && model.selectedParameterItem == null)
    { 
        methods = db.Methods.Where(f => f.MethodGroupID == selectedGroup).ToList();
        ViewBag.methods = convertMethod(methods);

        return View(methodModel);
    }

    else if(selectedGroup != 0 && model.selectedParameterItem != null)
    {
        List<Methods> groupMethods = db.Methods.Where(f => f.MethodID == selectedGroup).ToList();
        foreach (var i in model.selectedParameterItem)
        {
            parameters.Add(db.ParametersMethods.Where(f => f.ID == i).FirstOrDefault());
        }
        List<Methods> parameterMethods = methodlogicclass.GetMethodsFromParameters(parameters);



        foreach(var methods1 in groupMethods)
        {
            foreach(var methods2 in parameterMethods)
            {
                if(methods1.ID == methods2.ID)
                {
                    methods.Add(method2);
                }
            }
        }

        ViewBag.methods = convertMethods(methods);

        return View(methodModel);
    }

So I'm trying to create a searchmenu, with multiple options that don't necessarily have to be selected. However, search results obviously have to meet every option selected.

What would be the best way to write code like this, with the least amount of duplication, while keeping into account that model.selectedParameterItem (for instance) can be null and thus can't be used. Right now the code isn't closed for modification at all, which is far from optimal.

WHat would be best practice in a case like this? Check for each element if it is null, then get the results for each of them seperatly, then merge all the results?

Code explained:

  1. Search by group => search for methods by group
  2. Search by parameter => search for methods by parameters
  3. Etc, more will be added
  4. When searching for both, only return the methods that meet both group and parameter requirement (and in the future, other requirements).
  5. Group can be empty (0), list of parameters can be empty (null).
  6. Future added requirements will be able to be empty (null)
\$\endgroup\$
  • \$\begingroup\$ First advice - use English language for naming, just like .NET do. I assume you are using German now. For people who don't speak German your code looks like obfuscated: foreach (var a in b) { foreach (var c in d) { x.Add(c); }}} \$\endgroup\$ – Sergey Berezovskiy Feb 8 '17 at 15:40
  • \$\begingroup\$ What are parameters? What is wvl? What GetWerkovormUitBlahBlah method does? \$\endgroup\$ – Sergey Berezovskiy Feb 8 '17 at 15:47
  • 3
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Feb 8 '17 at 16:26
  • 1
    \$\begingroup\$ You will need more information to get a true answer. What is db? Entity framework? What is parameters? A list of some class? What is method object? what is convertToMethods method? \$\endgroup\$ – CharlesNRice Feb 8 '17 at 16:33
  • 2
    \$\begingroup\$ If you add the context above the code your question will be noticed better. \$\endgroup\$ – Heslacher Feb 8 '17 at 16:34
1
\$\begingroup\$
  1. Use English language for naming. .NET uses English for naming. You have mix of languages, and code looks like obfuscated for those who don't speak German.
  2. Use descriptive names. I don't know German, but wvl, wek1, wek2 is definitely not very descriptive.
  3. You have mix of presentation-related and data access code. You should split this code.

So, the first part is your controller code. It should look like technical specification when you read it. Let's try to do that

var methods = GetMethods(selectedGroupId, model.selectedParameterItem);
var model = ConvertMethodsToModel(methods);
return View(model);

Pretty clean. Now lets think about GetMethods method. It's purpose is returning some methods depending on what was found in methods group and parameter methods. Btw it can be method of another class (e.g. some service)

private IEnumerable<Method> GetMethods(int groupId, IEnumerable<int> parameterIds)
{
   var none = Enumerable.Empty<Method>();
   var groupMethods = (groupId == 0) ? none : GetMethodsByGroup(groupId);
   var parameterMethods = (parameterIds == null) ? none : GetParameterMethods(parametrIds);

   if (!parameterMethods.Any())
       return groupMethods;

   if (!groupMethods.Any())
       return parameterMethods;

   var parameterMethodsFromGroup =
        from pm in parameterMethods
        join gm in groupMethods on pm.Id equals gm.Id
        select pm;

   return parameterMethodsFromGroup;
}

And last part - those GetMethodsByGroup and GetParameterMethods (which could be moved to corresponding repositories):

private IEnumerable<Method> GetMethodsByGroup(int groupId)
{
    return db.Methods.Where(m => m.MethodGroupID == groupId).ToList();
}

Getting parameter methods:

private IEnumerable<Method> GetParameterMethods(IEnumerable<int> parameterIds)
{
    var parameterMethods = db.ParametersMethods.Where(pm => parameterIds.Contains(pm.ID));
    return methodlogicclass.GetMethodFromParameters(parameterMethodss).ToList();
}
\$\endgroup\$
  • \$\begingroup\$ The GetMethodsByGroup is calling ToList internally so I think it's better to return an IList so that the user knows he doesn't have to call it agian himself. It's always tempting to do it if a method returns an IEnumerable especially that Any is used and then agian the result is used in a query. You would get the possible multiple enumerations warning from ReSharper. The same applies to GetParameterMethods. \$\endgroup\$ – t3chb0t Feb 9 '17 at 8:11
  • \$\begingroup\$ @t3chb0t well, agree. But I rather go with removing ToList() from data access methods. Or returning Collection \$\endgroup\$ – Sergey Berezovskiy Feb 9 '17 at 10:40
  • \$\begingroup\$ Either way is actually fine but... by removing ToList it might happen that the db (probably context) is out of scope and already disposed so an exception will occur. But 2 - since we don't know the exact details let's say it's ok to return an IEnumerable ;-] \$\endgroup\$ – t3chb0t Feb 9 '17 at 10:45
1
\$\begingroup\$

if you are going to keep your current code, I would probably change the order of the if statements a little bit, and maybe separate some of the conditions out, definitely re order, the odd one out is model.selectedParameterItem == null && selectedGroup != 0 I would make that the first statement, like this

if (model.selectedParameterItem == null && selectedGroup != 0)
{
    methods = db.Methods.Where(f => f.MethodGroupID == selectedGroup).ToList();
    ViewBag.methods = convertMethod(methods);

    return View(methodModel);
}
else if (model.selectedParameterItem != null && selectedGroup == 0)
{
    foreach (var i in model.selectedParameterItem)
    {
        parameters.Add(db.ParametersMethods.Where(f => f.ID == i).FirstOrDefault());
    }
    methods.AddRange(methodlogicclass.GetMethodFromParameters(parameters));
    ViewBag.methods = convertMethod(method);

    return View(methodModel);       
}
else if (model.selectedParameterItem != null && selectedGroup != 0)
{
    List<Methods> groupMethods = db.Methods.Where(f => f.MethodID == selectedGroup).ToList();
    foreach (var i in model.selectedParameterItem)
    {
        parameters.Add(db.ParametersMethods.Where(f => f.ID == i).FirstOrDefault());
    }
    List<Methods> parameterMethods = methodlogicclass.GetMethodsFromParameters(parameters);

    foreach(var methods1 in groupMethods)
    {
        foreach(var methods2 in parameterMethods)
        {
            if(methods1.ID == methods2.ID)
            {
                methods.Add(method2);
            }
        }
    }

    ViewBag.methods = convertMethods(methods);

    return View(methodModel);       
}

and actually I was thinking that you should increase the indentation one level inside of your else if block, because of the conditions... the selectedGroup condition is the only thing different, no need to check the model.selectedParameterItem != null condition a third time.

if (model.selectedParameterItem == null && selectedGroup != 0)
{
    methods = db.Methods.Where(f => f.MethodGroupID == selectedGroup).ToList();
    ViewBag.methods = convertMethod(methods);

    return View(methodModel);
}
else if (model.selectedParameterItem != null) //&& selectedGroup == 0)
{
    if (selectedGroup == 0)
    {
        foreach (var i in model.selectedParameterItem)
        {
            parameters.Add(db.ParametersMethods.Where(f => f.ID == i).FirstOrDefault());
        }
        methods.AddRange(methodlogicclass.GetMethodFromParameters(parameters));
        ViewBag.methods = convertMethod(method);

        return View(methodModel);       
    }
    else if (selectedGroup != 0)
    {
        List<Methods> groupMethods = db.Methods.Where(f => f.MethodID == selectedGroup).ToList();
        foreach (var i in model.selectedParameterItem)
        {
            parameters.Add(db.ParametersMethods.Where(f => f.ID == i).FirstOrDefault());
        }
        List<Methods> parameterMethods = methodlogicclass.GetMethodsFromParameters(parameters);

        foreach(var methods1 in groupMethods)
        {
            foreach(var methods2 in parameterMethods)
            {
                if(methods1.ID == methods2.ID)
                {
                    methods.Add(method2);
                }
            }
        }   
        ViewBag.methods = convertMethods(methods);
        return View(methodModel);       
    }
}

I would also suggest using LINQ in the rest of your code, you use it sparingly throughout but not everywhere that you can. it would make the code a lot cleaner and get rid of nesting the foreach loops as well.

Your code here:

foreach (var methods1 in groupMethods)
{
  foreach (var methods2 in parameterMethods)
  {
      if (methods1.ID == methods2.ID)
      {
          methods.Add(method2);
      }
  }
}

would look a lot better like this

methods.AddRange(groupMethods.Select(gm => parameterMethods.First(pm => pm.ID == gm.ID)));
\$\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.