5
\$\begingroup\$

I have a block of code that pulls the current menu position and compares it to what users selected, if the selection is different the database is updated with the new selection. As I am still new with EF I was hoping for a general code review for suggestions on a possible better, more efficient, and/or easier way to code this. Here is my existing code:

using (var db = new SectionInfoAdmin())
{
    var dtCategories = (from n in db.Navigation
    where n.SectionID == SectionID
    select new
    {
       n.Category
    }).ToList();

 string PrimaryCategory = dtCategories[0].Category.ToString();

 if (ddlPrimaryCategory.SelectedValue != PrimaryCategory)
 {
    var Category = (from n in db.Navigation
    where n.Category == PrimaryCategory && n.SectionID == SectionID
       select n).First();

    Category.Category = PrimaryCategory;
 }

 if (dtCategories.Count > 1)
 {
    string SecondaryCategory = dtCategories[1].Category.ToString();
    if (ddlSecondaryCategory.SelectedValue == SecondaryCategory)
    {
       var Category = (from n in db.Navigation
       where n.Category == SecondaryCategory && n.SectionID == SectionID
          select n).First();
       Category.Category = SecondaryCategory;
    }
 }
 db.SaveChanges();
 lblResults.Text = "Information updated.";
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Let me throw something at you: why only save if there is a difference? Why not just save anyway? Unless you have database congestion (table locking etc) then a database access is cheap (EF generates efficient code), and this way you avoid complexity. Some would say "but why hit the database if you don't have to?". The answer is "Why not - especially if the decision to do so is computationally expensive, risky, or just unnecessary". \$\endgroup\$ – slugster Apr 19 '13 at 23:12
  • \$\begingroup\$ I am used to working directly against the DB and keep forgeting EF is more effeciant in the way it generates it code. Also did not know about codereview site, will keep that in mide for the feature. \$\endgroup\$ – Matthew Verstraete Apr 20 '13 at 0:02
  • \$\begingroup\$ I would start by splitting common code into methods. For example your Linq to get your category could be moved into it's own method. \$\endgroup\$ – dreza Apr 21 '13 at 1:46
8
\$\begingroup\$

As @dreza mentioned, you might want to move the code around your LINQ-Statements to a method.

public void changeCategoryTo(String newCategory){
   var record = (from n in db.Navigation where n.Category == newCategory
                   && n.SectionId == SectionID select n).First();
   record.Category = newCategory;
}

this greatly reduces duplication.

further points:

  1. Try to stick to either camelCase or MixedCase to have consistent and speaking variable names:

    • something like var Category, String PrimaryCategory and similar are diffifult to associate to clear roles in your code.
    • try to refrain from naming your variables after their type only, this leads to greatly increased effort when handling multiple objects of the same type.
  2. Your if-statement for updating the SecondaryCategory is different from the if-statement for updating PrimaryCategory

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy