current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 10 down vote favorite
1

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;
}
share|improve this question
1  
What type are the items of db.sumvanta ? –  Heslacher Sep 1 at 14:01
    
db.sumvanta is an sql table will strings and datetime cells mostly. –  George Sep 1 at 14:02
    
so var a in db.sumvanta refers to a DataRow ? –  Heslacher Sep 1 at 14:03
    
exactly a holds the name of the cells and i'm passing them on an object, but that's for another post! –  George Sep 1 at 14:04
    
Please do not edit "thanks" into a post. Just upvotes will do. –  Jamal♦ Sep 2 at 7:24

4 Answers 4

up vote 9 down vote accepted

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.

share|improve this answer
    
Almost everything is fine but the lengthy condition inside the if is a no-go ;-] –  t3chb0t Sep 1 at 16:05
    
Not as lengthy, as original if chain though ^^ –  Nikita Brizhak Sep 2 at 6:38
    
Thank you for your answer, I saw you updated and added the continue keyword. Why is that? It works without it. –  George Sep 2 at 7:20
1  
Personal preference. Feel free to use the version you like, they do exactly the same thing. :) –  Nikita Brizhak Sep 2 at 7:29
up vote 16 down vote

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));
}
share|improve this answer
up vote 9 down vote

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.

share|improve this answer
up vote 9 down vote

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);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.