Generic methods and Switch by type

[Ethan Strauss]

Hi,
I have written a generic method which does different things depending on
the type of the parameter. I got it to work, but it seems really inelegant.
Is there a better way to do this?
In the code below, ColumnMap is a simple struct which basically has three
properties, Header (a string), Index (an int), TypeOfData (which is a
DataType which is a local eNum). _Mapping is a local List<ColumnMap>.


public ColumnMap GetMap<T>(T value)
{
Type ThisType = typeof(T);
foreach (ColumnMap ThisMap in _Mapping)
{
if (ThisType == typeof(string))
{
if (ThisMap.Header.ToUpper() ==
value.ToString().ToUpper())
{
return ThisMap;
}
}
if (ThisType == typeof(int))
{
if (ThisMap.Index == int.Parse(value.ToString()))
{
return ThisMap;
}

}
if (ThisType == typeof(DataType))
{
if (ThisMap.TypeOfData.ToString() == value.ToString())
{
return ThisMap;
}
}
}
return new ColumnMap();
}
}


I would have prefered to do my comparisons by casting value to string, int,
ot DataType, but that gave me "cannot convert Type 'T' to ____" errors when
compiling.

I know could have written this as three overloaded methods with different
signatures, but I thought using a single Generic method would be easier.

Thanks for any thoughts!
Ethan

Ethan Strauss Ph.D.
Bioinformatics Scientist
Promega Corporation
2800 Woods Hollow Rd.
Madison, WI 53711
608-274-4330
800-356-9526
(e-mail address removed)

 

[Jeroen Mostert]

Hi,
I have written a generic method which does different things depending on
the type of the parameter. I got it to work, but it seems really inelegant.
Is there a better way to do this?
In the code below, ColumnMap is a simple struct which basically has three
properties, Header (a string), Index (an int), TypeOfData (which is a
DataType which is a local eNum). _Mapping is a local List<ColumnMap>.


public ColumnMap GetMap<T>(T value)
{
Type ThisType = typeof(T);
foreach (ColumnMap ThisMap in _Mapping)
{
if (ThisType == typeof(string))
{
if (ThisMap.Header.ToUpper() ==
value.ToString().ToUpper())

Don't do this, use one of the overloads of String.Equals() instead.

{
return ThisMap;
}
}
if (ThisType == typeof(int))
{
if (ThisMap.Index == int.Parse(value.ToString()))
{
return ThisMap;
}

}
if (ThisType == typeof(DataType))
{
if (ThisMap.TypeOfData.ToString() == value.ToString())
{
return ThisMap;
}
}
}
return new ColumnMap();
}
}


I would have prefered to do my comparisons by casting value to string, int,
ot DataType, but that gave me "cannot convert Type 'T' to ____" errors when
compiling.

Generics are supposed to promote type safety. Casting them to arbitrary
types makes no sense. You would actually have been better off if you'd
simply let this function take an Object.

I know could have written this as three overloaded methods with different
signatures, but I thought using a single Generic method would be easier.

When you found out you were wrong, why didn't you reverse your decision?
They really *are* three separate methods, and "type" is not sufficient to
distinguish them: you're not looking up a ColumnMap "by string", you're
looking it up "by header". You're not looking it up "by int", you're looking
it up "by index". So make the methods reflect that (this uses C# 3.0):

public ColumnMap GetMapByHeader(string header) {
return _Mapping.Where(m => string.Equals(m.Header, header,
StringComparison.InvariantCultureIgnoreCase)).FirstOrDefault();
}

public ColumnMap GetMapByIndex(int index) {
return _Mapping.Where(m => m.Index == index).FirstOrDefault();
}

public ColumnMap GetMapByTypeOfData(DataType dataType) {
return _Mapping.Where(m => m.TypeOfData == dataType).FirstOrDefault();
}

If you want to you can name these all the same; I wouldn't, but it's a
personal taste.

Note also that it's poor form to make a function that's nominally intended
to get an existing item create a new one when it can't find the value
supplied. Are clients supposed to check for existence first if they want to
find out if the value is present at all, or do default ColumnMaps have some
sort of .IsActuallyNotValid property?

Finally, clients will presumably know what ColumnMaps they want to find
themselves. Instead of offering separate methods for searching for them,
offer them access to the collection itself:

public IList<ColumnMap> Mappings {
get { return _Mappings.AsReadOnly(); }
}

This property can be queried in any way the client wants. If you do want to
offer specialized lookup, consider creating a new ColumnMappingCollection
class instead.

 

[Ethan Strauss]

Thanks for you reply Jeroen. I have some comments on your comments
Ethan

Don't do this, use one of the overloads of String.Equals() instead.

Why not? Not that I object to String.Equals(), but I would think that the
amount of extra work the CPU has to do would be pretty trivial. Am I wrong?

Generics are supposed to promote type safety. Casting them to arbitrary
types makes no sense. You would actually have been better off if you'd
simply let this function take an Object.

Not a bad idea. I may do that....

When you found out you were wrong, why didn't you reverse your decision?
They really *are* three separate methods, and "type" is not sufficient to
distinguish them:

You are part of the process of deciding what to do now. Going to 3 seperate
methods may be what I do, but I wanted to see if I was missing something
first.

Note also that it's poor form to make a function that's nominally intended
to get an existing item create a new one when it can't find the value
supplied. Are clients supposed to check for existence first if they want to
find out if the value is present at all, or do default ColumnMaps have some
sort of .IsActuallyNotValid property?

I do expect that clients will check for existence first, but if I leave this
out, then I get a "not all paths return a value" type error. Would you prefer
throwing an error here? If so, why?

Finally, clients will presumably know what ColumnMaps they want to find
themselves. Instead of offering separate methods for searching for them,
offer them access to the collection itself:

public IList<ColumnMap> Mappings {
get { return _Mappings.AsReadOnly(); }
}

This property can be queried in any way the client wants. If you do want to
offer specialized lookup, consider creating a new ColumnMappingCollection
class instead.

I have considered this and I think it is probably the way to go. I will
think some more on it and see what I come up with




Ethan

 

[Jeroen Mostert]

Why not? Not that I object to String.Equals(), but I would think that the
amount of extra work the CPU has to do would be pretty trivial. Am I wrong?

The problem is that if you use .ToUpper(), it's not clear what sort of
comparison you're intending. The default comparison is culture-specific,
which is rarely appropriate and leads to such classical problems as the one that

"i".ToUpper() == "I".ToUpper()

is *false* when the locale is Turkish (the so-called "Turkish i problem"),
because Turkish has two distinct letter i's (with and without dot).
String.Equals() avoids these issues. So do .ToUpper() with an appropriate
argument and .ToUpperInvariant(), but if you have the opportunity to do an
efficient comparison instead of producing more strings, why waste it? It's
still one line of code.

I do expect that clients will check for existence first, but if I leave this
out, then I get a "not all paths return a value" type error. Would you prefer
throwing an error here? If so, why?

There are plenty of patterns here:

- Provide GetValue() and have it return null if the specified value isn't
there. For this to work, null must not be a valid item in the collection.
This is by far the easiest approach. Clients have little opportunity for
failing by not checking the return value, because any call on the resulting
object will throw a NullReferenceException. It's also easy to propagate null
values to callers upstream that use the same convention.

- As a refinement of this, you can return a "null object" instead. This
object is an actual object (not a null reference) which simply provides a
do-nothing or trivial implementations of every method. This can remove
checking of edge cases and simplify logic in some cases, but the downside is
that it complicates handling for clients that are only interested in "real"
objects. Also, some methods may simply not allow for a meaningful null
implementation.

- Provide HasValue() and GetValue() and have GetValue() throw an exception.
This works, but it means getting a value if you don't know it exists is
twice as expensive (GetValue() will presumably search for the value in
exactly the same way HasValue() does). It also means that if this collection
is used in a concurrent scenario, clients *must* lock on the collection for
every single access, because there's no atomic way of retrieving a value
successfully.

- Provide GetValue(), which throws an exception if the item is not there,
and bool TryGetValue(out value), which sets value and returns a boolean
indicating whether the item was found. This is the generalization of
solution #1, used by (among others) Dictionary<T>. This works even if null
is an item in the collection, but TryGetValue() is cumbersome to call, and
this can get annoying especially if the common case is *not* knowing whether
an item is present.

Which one to use depends on your scenario. There's another approach that,
while often seen, I would rarely consider useful:

- Provide only GetValue() and have it throw an exception for values that
aren't there. This is only appropriate if clients *must* know a value exists
and any failure to know this is to be considered a bug, because "exceptions
should be exceptional". The question to ask here is: how can clients
*definitely know* the item is there unless they know someone else who put it
there? If they do, why didn't that party simply pass them the item instead
of making them retrieve it?

 

[Jon Skeet [C# MVP]]

Why not? Not that I object to String.Equals(), but I would think that the
amount of extra work the CPU has to do would be pretty trivial. Am I wrong?

Yes. Try comparing "MAIL" and "mail" with your code, but when you're
running in a Turkish locale.

If you want a case-insensitive comparison, do one - upper casing and
then doing an ordinal comparison is inherently culture-sensitive, and
can give inappropriate results.

(It also avoids creating extra strings for no good reason.)