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.

I am trying to use the following form to enter new info into a database.

enter image description here

I am trying to use the entity framework.

I have the following classes of Interest:

public class InventoryContext : DbContext
{
    public DbSet<Item> Items { get; set; }

    ....Other DbSets follow...
}


[Table("Items")]
public class Item
{
    #region Strings
    public string Color { get; set; }
    public string FullName { get; set; }
    [Column(@"Sheet/Roll")]
    public string Type { get; set; }
    public string PrimaryMachine { get; set; }
    public string Alias { get; set; }
    public string Brand { get; set; }
    public string Finish { get; set; }

    #endregion

    #region Long
    public long ID { get; set; }
    public decimal? Weight { get; set; }
    #endregion

    #region Doubles
    public decimal? Size1 { get; set; }
    public decimal? Size2 { get; set; }
    public decimal? Size3 { get; set; }
    #endregion.
}

And I am stuck on filtering the dgvAllItems, based on the selected drop down values. I think my code is ugly and there is a better way.

    private void ComboBox_SelectedIndexChanged(object sender, EventArgs e)
    {

        ComboBox s = (ComboBox)sender;

        IQueryable<Item> query = c.Items;

        foreach (ComboBox cb2 in gbFilters.Controls.OfType<ComboBox>().Where(com => com.Text != ""))
        {
            string currentpropname = cb2.Name.Substring(2);

            if (cb2.Name.Substring(2, 4) == "Size" || cb2.Name == "cbWeight")
            {
                decimal? currentpropvalue = Convert.ToDecimal(cb2.Text);
                PropertyInfo propertyInfo = typeof(Item).GetProperty(currentpropname);
                ParameterExpression pe = Expression.Parameter(typeof(Item), "e");
                MemberExpression me = Expression.MakeMemberAccess(pe, propertyInfo);
                ConstantExpression ce = Expression.Constant(currentpropvalue, typeof(decimal?));
                BinaryExpression be = Expression.Equal(me, ce);

                Expression<Func<Item, bool>> lambda = Expression.Lambda<Func<Item, bool>>(be, pe);
                query = query.Where(lambda);
            }
            else
            {
                string currentpropvalue = cb2.Text;
                PropertyInfo propertyInfo = typeof(Item).GetProperty(currentpropname);
                ParameterExpression pe = Expression.Parameter(typeof(Item), "e");
                MemberExpression me = Expression.MakeMemberAccess(pe, propertyInfo);
                ConstantExpression ce = Expression.Constant(currentpropvalue, typeof(decimal?));
                BinaryExpression be = Expression.Equal(me, ce);

                Expression<Func<Item, bool>> lambda = Expression.Lambda<Func<Item, bool>>(be, pe);
                query = query.Where(lambda);
            }

        }

        bindingSource1.DataSource = query.ToList();
    }

    private void ComboBox_DropDown(object sender, EventArgs e)
    {
        ComboBox cb1 = (ComboBox)sender;

        IQueryable<Item> query = c.Items;

        foreach (ComboBox cb2 in gbFilters.Controls.OfType<ComboBox>().Where(com => com.Text != ""))
        {
            string currentpropname = cb2.Name.Substring(2);

            if (cb2.Name.Substring(2, 4) == "Size" || cb2.Name == "cbWeight")
            {
                decimal? currentpropvalue = Convert.ToDecimal(cb2.Text); 
                PropertyInfo propertyInfo = typeof(Item).GetProperty(currentpropname);
                ParameterExpression pe = Expression.Parameter(typeof(Item), "e");
                MemberExpression me = Expression.MakeMemberAccess(pe, propertyInfo);
                ConstantExpression ce = Expression.Constant(currentpropvalue, typeof(decimal?));
                BinaryExpression be = Expression.Equal(me, ce);

                Expression<Func<Item, bool>> lambda = Expression.Lambda<Func<Item, bool>>(be, pe);
                query = query.Where(lambda);
            }
            else
            {
                string currentpropvalue = cb2.Text;
                PropertyInfo propertyInfo = typeof(Item).GetProperty(currentpropname);
                ParameterExpression pe = Expression.Parameter(typeof(Item), "e");
                MemberExpression me = Expression.MakeMemberAccess(pe, propertyInfo);
                ConstantExpression ce = Expression.Constant(currentpropvalue, typeof(decimal?));
                BinaryExpression be = Expression.Equal(me, ce);

                Expression<Func<Item, bool>> lambda = Expression.Lambda<Func<Item, bool>>(be, pe);
                query = query.Where(lambda);
            }

        }

        string ActivePropName = cb1.Name.Substring(2);

        if (ActivePropName.Substring(0, 4) == "Size" || ActivePropName == "Weight")
        {

            ParameterExpression arg = Expression.Parameter(typeof(Item), "x");
            Expression expr = Expression.Property(arg, ActivePropName);
            LambdaExpression lambda = Expression.Lambda(expr, arg);
            Expression<Func<Item, decimal?>> expression = (Expression<Func<Item, decimal?>>)lambda;

            cb1.DataSource = query.Select(expression).Distinct().ToList();
        }
        else
        {
            ParameterExpression arg = Expression.Parameter(typeof(Item), "x");
            Expression expr = Expression.Property(arg, ActivePropName);
            LambdaExpression lambda = Expression.Lambda(expr, arg);
            Expression<Func<Item, string>> expression = (Expression<Func<Item, string>>)lambda;

            cb1.DataSource = query.Select(expression).Distinct().ToList();
        }
    }
share|improve this question

1 Answer 1

I'm about to watch Vikings so here are a few quick pointers:

Naming

Your code contains variables named me, pe, ce, etc. This tells me nothing about what that variable does.

I'm not experienced with Expressions but I assume Expression.Equal refers to an == invocation. Such a variable could then be rewritten as equalsCondition, for example. This tells me a lot more about will also read easier when done everywhere:

expression = equalsCondition(itemValue, constantValue)

Duplication

Your if statements are very similar to eachother; similar to the point that I'm pretty sure you copy-pasted most of it. This is a sign that you need to factor it out to a method.

An example could be this:

private Expression<Func<Item, bool>> lambda GenerateQuery(object propertyValue)
{
    PropertyInfo propertyInfo = typeof(Item).GetProperty(currentpropname);
    ParameterExpression pe = Expression.Parameter(typeof(Item), "e");
    MemberExpression me = Expression.MakeMemberAccess(pe, propertyInfo);
    ConstantExpression ce = Expression.Constant(currentpropvalue, typeof(decimal?));
    BinaryExpression be = Expression.Equal(me, ce);
    Expression<Func<Item, bool>> lambda = Expression.Lambda<Func<Item, bool>>(be, pe);
}

and used as such:

if (cb2.Name.Substring(2, 4) == "Size" || cb2.Name == "cbWeight")
{
    decimal? currentpropvalue = Convert.ToDecimal(cb2.Text); 
    query = query.Where(GenerateQuery(currentpropvalue));
} else {
    string currentpropvalue = cb2.Text;
    query = query.Where(GenerateQuery(currentpropvalue));
}

If you push these two changes through to the several places they manifest, you'll find that your code has shrunk a lot and is a lot more readable.

share|improve this answer
    
Most of this is psuedo code for my actual program it does follow all standard naming converntion, Underscores, upper lower cases, comments and so. I actually extracted this code out of the methods to make it more clear in my sample (For me trying to follow definitions and references through code on here is a lot harder then just having it in there. But none of this actually helps with the question as I am more looking for performance enhancement or better code conventions ie. not use the field names as the name of the controls how to split my code into generic –  user2125348 Apr 28 at 21:14
    
classes and not have multiple overloaded methods. Or entire new concepts that maybe avoid the entity framework or allow me to use it in a better way. As I am about 80% sure I am not using it as it was intended, but as I don't know how it was intended I can't tell. –  user2125348 Apr 28 at 21:15
1  
Sure, I don't see any repositories and you're accessing your datasource (through EF) from a GUI method. For a quick review on EF & repositories, look here. Please use the exact code in your question next time, as is requested in the Help Center. –  Jeroen Vannevel Apr 28 at 21:56
    
My point was that I wasn't asking for how to format my code better, but how to make it perform better. Maybe It is not the correct type of question for the site? Or I asked incorrectly. –  user2125348 Apr 28 at 21:59
1  
@user2125348 it is - except answerers on this site are free to address anything they see in your code, about any facet one might have thought of.. or not. You may ask about specific performance concerns, so reviewers try to steer their attention in that direction. But if I read code and I'm having a problem with its readability, you can be sure I'm going to make a note about readability in my review! Performance is one thing, but if you're focused on performance and someone points out an important maintainability issue, it's a plus! ;) –  Mat's Mug Apr 29 at 1:24

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.