1

Dont know what object type will be passed in, if it is type of FishDTO it has a different route to generate the uri, if it is not fish than it is mammal and should follow standard route. Is there a cleaner way to do this? Instead of using type is there a better way using as? This works but it just looks ugly:

public Uri GetAnimalImageUrl(object animalResult)
{
    if (animalResult.GetType() == typeof(FishDTO))
    {
        return new Uri(GetFishImageUrl((FishDTO)animalResult));
    }

    var mammal = (MammalDTO)animalResult;
    var mammalUrl = GetMammalImageUrl(mammal);
    if (!string.IsNullOrEmpty(mammalUrl))
    {
        return new Uri(mammalUrl);
    }

    _logger.Error("Image not found for animal");
    return null;
}
1
  • Make a method that accepts a FishDTO. Make a different method that accepts a MammalDTO. Do those classes have an inheritance relationship? If so you only need one method...
    – user47589
    Jan 23, 2020 at 18:51

1 Answer 1

6

First off, never do this:

    if (animal.GetType() == typeof(Fish))

Why not? Because what if animal is a Goldfish, a derived type of Fish? GetType will return typeof(Goldfish). Instead, always do this:

    if (animal is Fish fish)

because that is true regardless of whether animal is Fish or Goldfish or Shark.

Second, your code assumes that anything that is not a fish is a mammal, but I think the birds, lizards and crustaceans would like to disagree with you. Your code is not robust in the face of unexpected inputs.

The real question to answer here is why is the formal parameter of type object in the first place? That seems like the problem to fix. If that problem is unfixable, then do your best to fix it:

Uri HandleFish(Fish fish) ...
Uri HandleMammal(Mammal mammal) ...
Uri HandleAnimal(Animal animal)
{
  if (x is Fish fish) return HandleFish(fish);
  if (x is Mammal mammal) return HandleMammal(mammal);
  ...
  return HandleOther(x);
}
Uri HandleOther(object x) ...
Uri HandleObject(object x)
{
  if (x is Animal animal) return HandleAnimal(animal);
  ...
  return HandleOther(x);
}

Now at least you are working with the type system.

Or, in newer versions of C# you can say

switch(x)
{
  case Fish fish: return HandleFish(fish);
  ...

An alternative approach is to put the logic in the type hierarchy itself.

class Animal
{
  public virtual Uri Handle() ...
}
class Fish : Animal
{
  public override Uri Handle() ...
}

And now if x is Animal then you just return ((Animal)x).Handle());

11
  • 1
    @UweKeim: Was in the process of making that edit when you commented! I am not in the habit yet of using all the pattern features. :) Jan 23, 2020 at 19:02
  • 1
    @Amy: I, like many compiler writers, am in the habit of referring to formal parameters as "formals". I'll fix it. Jan 23, 2020 at 19:08
  • 1
    @Amy: I note that we often do this in other contexts. In Canada the high school dance that Americans call "the prom" is called "the formal", and of course there are many other situations where adjectives are nominalized in English. Jan 23, 2020 at 20:17
  • 1
    @EricLippert Indeed. I just didn't know this particular jargon. Now I do.
    – user47589
    Jan 23, 2020 at 20:41
  • 1
    @EricLippert - just did a minor edit to your code example, hope that was okay. Jan 23, 2020 at 22:31

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.