3
\$\begingroup\$

Does this extension method contain unnecessary work? - Thanks for reviewing

Scenario - A series of tables are Key-Value pairs (Int "Id" / nvarChar "Title"). This extension method "dots-off" any integer ("Id") and provide the corresponding string value ("Title") given a target "type"

Used in aspx-code-behind. An extension method was chosen for readable one-liners. There are many panels with similar controls to be set conditionally.

// aspx.cs
txtParentGender.Text=parent.GenderId.RhGet(typeof(Gender));  //male, female
txtParentEthnicity.Text=parent.EthnicityId.RhGet(typeof(Ethnicity)); //Caucasian, etc. 
txtParentMaritalStatus.Text=parent.MaritalStatusId.RhGet(typeof(MaritalStatus)); // married, divorced etc.
txtChildGender.Text=child.GenderId.RhGet(typeof(Gender));
txtChildEthnicity.Text=child.EthnicityId.RhGet(typeof(Ethnicity));
txtChildRelation.Text=child.RelationId.RhGet(typeof(Relation)); //son, daughter etc.


// class.cs
using System.Linq.Dynamic; // nuget package

public static string RhGet(this int id, Type type )
{

   string returnValue = string.Empty;

   // set up data context
   using (MyDataContext dc = new MyDataContext())
   {
       // get table corresponding to this type
       var table = dc.GetTable(type);

       // match and select record using "id"
       var matchingRecord = ((IQueryable)table).Where(string.Format("Id.Equals({0})", id))
                                               .Select("new(Title)").Cast<object>().FirstOrDefault();

       // do a lot of contorting to finally get the string value
       returnValue = matchingRecord.GetType().GetProperty("Title").GetValue(matchingRecord,null).ToString();
    }

    return returnValue;
}
\$\endgroup\$
  • \$\begingroup\$ Does this code even work? Can you link the documentation of IQueryable.Where(string) and IQueryable.Select(string)? I'm not seeing it: msdn.microsoft.com/en-us/library/vstudio/… \$\endgroup\$ – Jeroen Vannevel Apr 13 '15 at 20:28
  • \$\begingroup\$ Hi @JeroenVannevel it's using DynamicLinq which accepts literal string expressions \$\endgroup\$ – Neville Apr 13 '15 at 20:32
  • \$\begingroup\$ You might also want to include what you're trying to accomplish with this method. Is there a reason you aren't comfortable with straightforward L2S syntax like table.Single(e => e.Id == id).Title in your code? It's not problematic to have these types of queries sprinkled around your code without having a grand unifying method that everything with an ID/Title structure has to call, if you're worried about that. \$\endgroup\$ – moarboilerplate Apr 13 '15 at 21:16
3
\$\begingroup\$

I would not write this extension method for int at all since it assumes everything of type int in your project is referring to the ID of a table.

Also, as a rule of thumb, if you're using strings that contain structural metadata about your objects, it's generally a sign you might want to rethink your approach.

Instead, I would find a way to apply an interface to your tables that have this structure, so you could then make an extension method for that interface, or a method that takes in an instance that implements that interface. That way you don't have to rely on strings at all.

If you are really committed to having a method that finds a value based on an int and type inputs, I would change the method to be a static, non-extension method, passing in the int and type.

I also came up with this alternative solution. Although it doesn't behave the same as your original example, you might benefit by using a type parameter so you get a strongly-typed entity returned by your method, which your code will know has a "Title" property:

public static TEntity GetEntityById<TEntity>(int id)
{

   TEntity entity = null;

   //a using is a try finally. you have stuff you want to execute in a finally so I simplified.
   try
   {
        MyDataContext dc = new MyDataContext();

       // match and select record using "id"
       entity = dc.GetTable<TEntity>().Where(string.Format("Id.Equals({0})", id)).Cast<TEntity>().FirstOrDefault(); //not sure if Cast<TEntity> is redundant or not
    }
    finally 
    {
        return entity;
    }
}

//consuming code
string myTitle = GetEntityById<MyEntity>(222).Title;

I also noticed you are instantiating a datacontext in this method--you might want to either pass it in or make this an extension method on your datacontext for testability.

\$\endgroup\$
  • \$\begingroup\$ Thank you, interesting thoughts. I'm coming from a web-dev background so I don't have full understanding of interfaces. \$\endgroup\$ – Neville Apr 13 '15 at 20:38
  • \$\begingroup\$ One of the difficulties with L2S is that it makes it really hard to apply interfaces to your objects (you might have to create a T4 template, blah) so that's just my "if you can do it" suggestion. Or you could switch to Entity Framework which gives you a bit more flexibility. \$\endgroup\$ – moarboilerplate Apr 13 '15 at 20:45
  • \$\begingroup\$ I also just remembered you could create a generic method with a type parameter instead of passing the type in which will help you out with your reflection problems. Hold up a sec, I'll update my answer \$\endgroup\$ – moarboilerplate Apr 13 '15 at 20:46
  • \$\begingroup\$ @Neville updated. This should help you out as an alternative to trying to get interfaces around your tables. \$\endgroup\$ – moarboilerplate Apr 13 '15 at 21:06
  • 1
    \$\begingroup\$ @Neville I would recommend extending your DataContext instead of int. One, mental model. The DataContext is a common element that these tables can be classified under. Two, you can keep the management of the data context outside of this code. This keeps your method atomic, and also allows you to unit test this method by passing in a mocked DataContext (I'm not sure how friendly L2S is for that). Third, it's normally not a good idea to extend a built-in type like int because like I said, doing so is making the incorrect assumption that every int in your app is the ID of one of these tables. \$\endgroup\$ – moarboilerplate Apr 14 '15 at 15:46

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.