2
\$\begingroup\$

I have a grid which allows a user to enter a new Grid item. If the grid item exists or the tb is empty it displays the error. Here's the code I came up with.

Can anybody think of a cleaner or more efficient way of doing so? Thinking about it now, I guess I could have done it all in one function, and placed InsertTrack() in the else condition of DoesTrackExist and register the script in the if statement.

 private void InsertTrack()
   {
       if (DoesTrackExist(txtEventTracks.Text))
       {
           using (EMSEntities db = new EMSEntities())
           {
               EventTrack newEventTrack = new EventTrack()
               {
                   TrackName = txtEventTracks.Text
               };                   
                   db.EventTracks.Add(newEventTrack);
                   db.SaveChanges();                   
           }
       }
       else
       {
          ScriptManager.RegisterStartupScript(Page, typeof(Page), "Error", "alert('Track name cannot be duplicate or empty')", true);
       }
   }
   private bool DoesTrackExist(string txtValue)
   {
       using (EMSEntities db = new EMSEntities())
       {
           var context = from x in db.EventTracks
                         where x.TrackName.Contains(txtEventTracks.Text)
                         select x;

           if (context.Any() || txtEventTracks.Text == "")
           {
               return false;
           }
           else
           {
               return true;
           }
       }
   }
\$\endgroup\$
  • \$\begingroup\$ Malachi, I used your edit, but how can I had that code block when I post \$\endgroup\$ – CSharper Oct 3 '13 at 15:14
1
\$\begingroup\$

Some people say that adding private functions which are only called in one place in a class clutter the "private interface" of the class. However I actually do prefer it in some cases. In your case I think it makes sense as it makes InsertTrack easier to read so I would not merge the two.

As to DoesTrackExist

  1. Check txtEventTracks.Text == "" first - this avoids performing any query at all.
  2. I assume you meant to use the parameter txtValue instead of accessing the textbox directly.
  3. Might not be more efficient but Any() accepts a predicate as well:

    private bool DoesTrackExist(string txtValue)
    {
        // maybe even consider IsNullOrWhiteSpace?
        if (string.IsNullOrEmpty(txtValue)) 
           return false;
        using (EMSEntities db = new EMSEntities())
        {
            return db.EventTracks.Any(x => x.TrackName.Contains(txtValue))
        }
    }
    
\$\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.