11
\$\begingroup\$

I'm dealing with a searching query where the user will pick from menus and checkboxes the variables-criteria I'm passing to the toWeb method and a list will be returned. This works fine but I'm going to add two more variables and it's already hard to follow.

 public List<newEvent> toWeb(string name, string egk, string             
 typosSumvan, Boolean olaTaSumvanta)
 {
    List<newEvent> listaSumvantwn = new List<newEvent>();

    if (olaTaSumvanta == false)
    {
        if (egk != "")
        {
            if (typosSumvan != "")
            {
                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.apodektis == name && a.egkatastasi == egk && a.eidosSumvan == typosSumvan)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
            else
            {
                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.apodektis == name && a.egkatastasi == egk)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
        }
        else
        {
            if (typosSumvan != "")
            {
                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.apodektis == name && a.eidosSumvan == typosSumvan)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
            else
            {
                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.apodektis == name)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
        }
    }
    else
    {
        if (egk != "")
        {
            if (typosSumvan != "")
            {

                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.egkatastasi == egk && a.eidosSumvan == typosSumvan)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
            else
            {//anazitisi xwris tupo
                //anazitisi xwris tupo sumvan
                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.egkatastasi == egk)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
        }
        else
        {                        
            if (typosSumvan != "")
            {
                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        if (a.eidosSumvan == typosSumvan)
                        {
                            listaSumvantwn.Add(readFromTable(a));
                        }
                    }
                }
            }
            else
            {

                using (var db = new CMMSEntity())
                {
                    foreach (var a in db.sumvanta)
                    {
                        listaSumvantwn.Add(readFromTable(a));
                    }
                }
            }
        }
    }

    return listaSumvantwn;
} 
\$\endgroup\$
6
  • 1
    \$\begingroup\$ What type are the items of db.sumvanta ? \$\endgroup\$
    – Heslacher
    Sep 1, 2015 at 14:01
  • \$\begingroup\$ db.sumvanta is an sql table will strings and datetime cells mostly. \$\endgroup\$
    – Oh hi Mark
    Sep 1, 2015 at 14:02
  • \$\begingroup\$ so var a in db.sumvanta refers to a DataRow ? \$\endgroup\$
    – Heslacher
    Sep 1, 2015 at 14:03
  • \$\begingroup\$ exactly a holds the name of the cells and i'm passing them on an object, but that's for another post! \$\endgroup\$
    – Oh hi Mark
    Sep 1, 2015 at 14:04
  • \$\begingroup\$ Please do not edit "thanks" into a post. Just upvotes will do. \$\endgroup\$
    – Jamal
    Sep 2, 2015 at 7:24

4 Answers 4

10
\$\begingroup\$

Emmm... can't you just join your bool conditions? Or am I missing something?

//method names and class names should start with capital letter
public List<NewEvent> ToWeb(string name, string egk, string typosSumvan, Boolean olaTaSumvanta)
{
    List<NewEvent> listaSumvantwn = new List<NewEvent>();

    using (var db = new CMMSEntity())
    {
        foreach (var a in db.sumvanta)
        {
            if (!olaTaSumvanta && a.apodektis != name) continue;
            if (!String.IsNullOrEmpty(egk) && a.egkatastasi != egk) continue;
            if (!String.IsNullOrEmpty(typosSumvan) && a.eidosSumvan != typosSumvan)) continue;

            listaSumvantwn.Add(ReadFromTable(a));
        }
    }

    return listaSumvantwn;
}

Or using LINQ

public List<NewEvent> ToWeb(string name, string egk, string typosSumvan, Boolean olaTaSumvanta)
{
    using (var db = new CMMSEntity())
    {
        return db.sumvanta.Where(a => olaTaSumvanta || a.apodektis == name)
                          .Where(a => String.IsNullOrEmpty(egk) || a.egkatastasi == egk)
                          .Where(a => String.IsNullOrEmpty(typosSumvan) || a.eidosSumvan == typosSumvan)
                          .Select(a => ReadFromTable(a))
                          .ToList();
    }
}

Also you should consider using English language for naming purposes.

\$\endgroup\$
4
  • \$\begingroup\$ Almost everything is fine but the lengthy condition inside the if is a no-go ;-] \$\endgroup\$
    – t3chb0t
    Sep 1, 2015 at 16:05
  • \$\begingroup\$ Not as lengthy, as original if chain though ^^ \$\endgroup\$
    – Nikita B
    Sep 2, 2015 at 6:38
  • \$\begingroup\$ Thank you for your answer, I saw you updated and added the continue keyword. Why is that? It works without it. \$\endgroup\$
    – Oh hi Mark
    Sep 2, 2015 at 7:20
  • 1
    \$\begingroup\$ Personal preference. Feel free to use the version you like, they do exactly the same thing. :) \$\endgroup\$
    – Nikita B
    Sep 2, 2015 at 7:29
16
\$\begingroup\$

Each of your parameters is either adding a filter, or an ignored value. We can construct a list of Predicates, each one corresponding to a single parameter.

 public List<newEvent> toWeb(string name, string egk, string             
 typosSumvan, Boolean olaTaSumvanta)
 {
    var predicates = new List<Predicate<DataRow>>();
    if (!olaTaSumvanta) { predicates.Add(row => row.apodektis == name) }
    if (egk != "") { predicates.Add(row => row.egkatastasi == egk) }
    if (typosSumvan != "") { predicates.Add(row => row.eidosSumvan == typosSumvan) }

    using (var db = new CMMSEntity())
    {
        foreach (var a in db.sumvanta)
        {
            if (a.IsValidEntity(predicates))
            {
                listaSumvantwn.Add(readFromTable(a));
            }
        }
    }
}

Using an extension method taken from this answer, we then apply all the filters.

public static Boolean IsValidEntity<T>(this T entity, IEnumerable<Predicate<T>> predicates)
{
    return predicates.All(p => p(entity));
}
\$\endgroup\$
10
\$\begingroup\$

Take a step back and look at the structure of the code and look for a repeating pattern. If you do that, you will see that following:

using (var db = new CMMSEntity())
{
    foreach (var a in db.sumvanta)
    {
        if (SOME BOOLEAN EXPRESSION INVOLVING a)
        {
            listaSumvantwn.Add(readFromTable(a));
        }
    }
}

Extracting this into a sub function will greatly improve this code.

private void SomeGoodFunctionName(Func<TYPE OF a, bool> shouldAdd)
{
    using (var db = new CMMSEntity())
    {
        foreach (var a in db.sumvanta)
        {
            if (shouldAdd(a))
            {
                listaSumvantwn.Add(readFromTable(a));
            }
        }
    }
}

Which is then used like the following to never be nested more than 3 levels.

if (olaTaSumvanta == false)
{
    if (egk != "")
    {
        if (typosSumvan != "")
        {
            SomeGoodFunctionName(a => a.apodektis == name && a.egkatastasi == egk && a.eidosSumvan == typosSumvan);
        }

Since the boolean expressions are some what complicated, it would be better to create stand alone functions with names describing what is being checked. Then use that instead of a long lambda expression.

Note: In the last repeated block, there is no if block inside the foreach loop. This is equivalent to the boolean expression being true.

\$\endgroup\$
10
\$\begingroup\$

Let's assume the elements of the db.sumvanta are of type Record.
You could move creation of the resulting List to the method:

public static List<newEvent> GetRecords(Func<Record, bool> match)
{
    List<newEvent> listaSumvantwn = new List<newEvent>();
    using (var db = new CMMSEntity())
    {
        foreach (var a in db.sumvanta)
        {
            if (match(a))
            {
                listaSumvantwn.Add(readFromTable(a));
            }
        }
    }
    return listaSumvantwn;
}

Then your method turns into the following:

public List<newEvent> toWeb(string name, string egk, string typosSumvan, Boolean olaTaSumvanta)
{
    if (!olaTaSumvanta)
    {
        if (egk != "")
        {
            return typosSumvan != ""
                ? GetRecords(a => a.apodektis == name && a.egkatastasi == egk && a.eidosSumvan == typosSumvan)
                : GetRecords(a => a.apodektis == name && a.egkatastasi == egk);
        }
        return typosSumvan != ""
            ? GetRecords(a => a.apodektis == name && a.eidosSumvan == typosSumvan)
            : GetRecords(a => a.apodektis == name);
    }

    if (egk != "")
    {
        return typosSumvan != ""
            ? GetRecords(a => a.egkatastasi == egk && a.eidosSumvan == typosSumvan)
            : GetRecords(a => a.egkatastasi == egk);
    }
    return typosSumvan != ""
        ? GetRecords(a => a.eidosSumvan == typosSumvan)
        : GetRecords(a => true);
} 
\$\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.