0

In code review I have found interesting construction:

public ActionResult SeatMap(string name)
{
   var equimpment = this.CustomResourcesCache.GetAllEquipment(name);

   if (string.IsNullOrEmpty(name))
   {
      List<dynamic> all = new List<dynamic>();
      foreach (var item in equimpment)
      {
         all.Add(item.Tag[0]);
      }
      return CachedResult(new { SeatMaps = all } );
     }
     return CachedResult(new { SeatMaps = equimpment.Where(o => o.Key == name).Count()>0?equimpment.Where(o => o.Key == name).First().Tag:new List<dynamic>() });
   }

I am just wondering why they used List <<[dynamic]>> instead of List<<[T]>> and is there any other solution instead of this one.

Thanks

bane 975
  • 87
  • 1
  • 7
  • 2
    It seems to me that you need to learn about dynamic. Then you can answer to your question. – Dilshod Jul 10 '15 at 10:31
  • possible duplicate of [What is the 'dynamic' type in C# 4.0 used for?](http://stackoverflow.com/questions/2690623/what-is-the-dynamic-type-in-c-sharp-4-0-used-for) – Fabjan Jul 10 '15 at 10:32
  • 1
    @Dilshod I don't think you understood the question. It isn't about `dynamic` but why it was used at all. – Panagiotis Kanavos Jul 10 '15 at 10:36
  • The code is horrible altogether, but I think @Panagiotis is right about the lazy part. And the offtopic part as well. We can't know why the original developer chose to use dynamic. In practice you hardly ever need it. – CodeCaster Jul 10 '15 at 10:36
  • 3
    @PanagiotisKanavos Code explanation is off-topic for [CR.SE](http://codereview.stackexchange.com) – Mast Jul 10 '15 at 10:36
  • @PanagiotisKanavos I am not saying it is about dynamics. I said if you understood dynamics then you could have answered your own question. – Dilshod Jul 10 '15 at 10:39
  • I also suppose it was lazyness/whrong understannding on `dynamic`-keyword. You can however also write `List` – MakePeaceGreatAgain Jul 10 '15 at 10:39
  • @Dilshod I understand dynamics and I don't think there's a reason for this code. Laziness, or sloppy coding of `CachedResults` makes more sense – Panagiotis Kanavos Jul 10 '15 at 10:42
  • @Dilshod It seems to me that they used wrong data structure here. – bane 975 Jul 10 '15 at 10:52
  • 1
    I'm voting to close this question as off-topic because it's about explaining existing code, not fixing a problem. – Luaan Jul 10 '15 at 11:05

1 Answers1

2

Here's one rare scenario in which I would find this code acceptable.

Let's say GetAllEquipment returns a list of Equipment. Equipment does not contain the Key property that this code wants, but serves as a base class for various equipment classes, all of which have the Key property, but they don't share an interface. In that case, there is no T that you can put in List<T>.

Now, if you own the code for Equipment and its children, the solution would be to create a shared interface with Key inside (and use this interface as your T).

But if you don't own the code, like if it's a third party library, then using List<dynamic> seems acceptable, with the understanding that future changes to that library might break your code (i.e if they introduce a new child to Equipment which doesn't have the Key property).

tzachs
  • 4,331
  • 1
  • 28
  • 28