6
\$\begingroup\$

I have this code that I was asked to maintain, it is a notification system for a third party application event system (a lot more involved than that but for the sake of this question...)

The Code uses a class DataTable Variable with 4 columns, it takes one of the columns and takes the distinct values and outputs them to a list, but the person that wrote the code made it no so dynamic values are static.

What I am looking for is a way that the underlying DataTable will determine the List<string> instead of having to go through all these if statements that have the wonderful magic strings.

private List<string> GetBaseTypes()
{
    List<string> baseTypes = new List<string>();
    var result = (
       from myRow in _allEventsList.AsEnumerable()
       select myRow.Field<string>("BaseTypeDesc"))
       .Distinct();

    var item = result
       .SingleOrDefault(e => e == "Order");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Motion");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Service");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Original Filing Document");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Amended Filing Documents");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Case Event Text");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Other Event Types");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Attorney Assignment");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Court Administration");
    if (item != null)
        baseTypes.Add(item);
    item = result
       .SingleOrDefault(e => e == "Physical Filing Location");
    if (item != null)
        baseTypes.Add(item);

    return baseTypes;
}

Does anyone see something that sticks out where I could simplify this code?

I have been tasked with changing the data going into the DataTable and I don't want to enter in new entries every time something is added to the "BaseTypeDesc" .

\$\endgroup\$
5
\$\begingroup\$

Two code-style issues are immediately obvious:

  1. Use braces for 1-liners
  2. only split lines where it overflows

Code like:

var item = result
   .SingleOrDefault(e => e == "Order");
if (item != null)
    baseTypes.Add(item);
item = result
   .SingleOrDefault(e => e == "Motion");

Should be

var item = result.SingleOrDefault(e => e == "Order");
if (item != null)
{
    baseTypes.Add(item);
}
item = result.SingleOrDefault(e => e == "Motion");

Then, the logical thing to do to simplify those cascading statements, is to create an array of keys to look for (perhaps by loading from a configuration file, or as a static constant array...):

string[] keys = {"Order", "Motion", .....};

then loop through them like:

foreach (string key in keys)
{
    var item = result.SingleOrDefault(e => e == key);
    if (item != null)
    {
        baseTypes.Add(item);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I like this, but I still have to add new keys every time a new value is added to the Database Table that the DataTable is created from. \$\endgroup\$ – Malachi Dec 8 '14 at 17:50
  • 1
    \$\begingroup\$ That is true, in that case, you should consider loading the actual string values from a configuration file, or something. The current code gives the impression that the value order is significant, and that the distinct list of values from the database needs to be filtered in some way. The code I propose will be compatible with the existing code. \$\endgroup\$ – rolfl Dec 8 '14 at 18:00
  • \$\begingroup\$ That is a good point, this is the best answer for the question as I posted it. \$\endgroup\$ – Malachi Dec 8 '14 at 18:06
2
\$\begingroup\$
  • You should instead of returning a List <String> return an IEnumerable <String>.
  • to simplify this, I guess I don't understand it correctly, you could just return result.

As I am on my mobile, thats it.

\$\endgroup\$
  • \$\begingroup\$ I was just about to post what I did, and this explains it in one line...lol. I will probably post a follow up. \$\endgroup\$ – Malachi Dec 8 '14 at 17:11
2
\$\begingroup\$

Since it looks like you are just trying to find the intersection of a known set of keys and those present in the table/column, you could just use LINQ's Intersect method.

To facilitate this, I would probably pull the list of keys out to a "constant" field. Additionally, unless necessary to return a List<string>, as Heslacher suggests, I would return IEnumerable<string>.

private IEnumerable<string> GetBaseTypes()
{
    var result = (
       from myRow in _allEventsList.AsEnumerable()
       select myRow.Field<string>("BaseTypeDesc"))
       .Distinct();

    return result.Intersect(INTERSECT_KEYS).ToList();
}

private static readonly string [] INTERSECT_KEYS =
    new [] {"Order", "Motion", "Service", "Original Filing Document",
            "Amended Filing Documents", "Case Event Text",
            "Other Event Types", "Attorney Assignment",
            "Court Administration", "Physical Filing Location"};
\$\endgroup\$
2
\$\begingroup\$

Since you're already using LINQ, here's another option:

var validTypes = new HashSet<string> { "Order", "Motion", "Service", "....." };

var result = (
    from row in table.AsEnumerable() 
    let value = row.Field<string>("BaseTypeDesc")
    where validTypes.Contains(value)
    select value
).Distinct();

Removing the use of SingleOrDefault shouldn't change the nature of the code, since the Distinct on the LINQ already prevents any duplicate values. Also, by using a HashSet, this should be quite quick.

As to your primary concern, the list of valid types is now only in one line (or block) of code, rather than spread across the entire method. It can also easily be loaded from a configuration file, as rolfl suggested.

\$\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.