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\$
  • 1
    \$\begingroup\$ What type are the items of db.sumvanta ? \$\endgroup\$ – Heslacher Sep 1 '15 at 14:01
  • \$\begingroup\$ db.sumvanta is an sql table will strings and datetime cells mostly. \$\endgroup\$ – Oh hi Mark Sep 1 '15 at 14:02
  • \$\begingroup\$ so var a in db.sumvanta refers to a DataRow ? \$\endgroup\$ – Heslacher Sep 1 '15 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 '15 at 14:04
  • \$\begingroup\$ Please do not edit "thanks" into a post. Just upvotes will do. \$\endgroup\$ – Jamal Sep 2 '15 at 7:24
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\$
  • \$\begingroup\$ Almost everything is fine but the lengthy condition inside the if is a no-go ;-] \$\endgroup\$ – t3chb0t Sep 1 '15 at 16:05
  • \$\begingroup\$ Not as lengthy, as original if chain though ^^ \$\endgroup\$ – Nikita B Sep 2 '15 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 '15 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 '15 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.