20

I am trying to create a generic class which new's up an instance of the generic type. As follows:

public class HomepageCarousel<T> : List<T>
    where T: IHomepageCarouselItem, new()
{
    private List<T> GetInitialCarouselData()
    {
        List<T> carouselItems = new List<T>();

        if (jewellerHomepages != null)
        {
            foreach (PageData pageData in jewellerHomepages)
            {
               T item = new T(pageData); // this line wont compile
               carouselItems.Add(item);
            }
        }
        return carouselItems;
    }
}

But I get the following error:

cannot provide arguments when creating an instance of a variable type

I found the following related question which is very close to what I need: Passing arguments to C# generic new() of templated type

However, I can't used Jared's suggested answer as I am calling the method within the Generic class, not outside of it, so I can't specify the concrete class.

Is there a way around this?

I have tried the following based on the other question, but it doesn't work as I don't know the concrete type of T to specify. As it is called from inside the generic class, not outside:

public class HomepageCarousel<T> : List<T>
    where T: IHomepageCarouselItem, new()
{

    private List<T> LoadCarouselItems()
    {
        if (IsCarouselConfigued)
        {
            return GetConfiguredCarouselData();
        }

        // ****** I don't know the concrete class for the following line,
        //        so how can it be instansiated correctly?

        return GetInitialCarouselData(l => new T(l));
    }

    private List<T> GetInitialCarouselData(Func<PageData, T> del)
    {
        List<T> carouselItems = new List<T>();

        if (jewellerHomepages != null)
        {
            foreach (PageData pageData in jewellerHomepages)
            {
                T item = del(pageData);
                carouselItems.Add(item);
            }
        }
        return carouselItems;
    }
}

********EDIT : ADDED POSSIBLE SOLUTIONS**

So I have tested 2 possible solutions:

First is exactly as explained below by Jon Skeet. This definitely works but means having an obscure lambda in the constructor. I am not very comfortable with this as it means users need to know the correct lambda that is expected. After all, they could pass a lambda which doesn't new up the type, but does something entirely unexpected

Secondly, I went down the Factory method route; I added a Create method to the common interface:

IJewellerHomepageCarouselItem Create(PageData pageData);

Then provided an implementation in each Concrete class:

public IJewellerHomepageCarouselItem Create(PageData pageData)
{
     return new JewellerHomepageCarouselItem(pageData, null);
}

And used a two step initialisation syntax:

T carouselItem = new T();
T homepageMgmtCarouselItem = (T) carouselItem.Create(jewellerPage);

Would love to hear some feedback on the merit of each of these approaches.

Community
  • 1
  • 1
ChrisCa
  • 10,876
  • 22
  • 81
  • 118
  • possible duplicate of [Passing arguments to C# generic new() of templated type](http://stackoverflow.com/questions/840261/passing-arguments-to-c-sharp-generic-new-of-templated-type) – nawfal Apr 23 '13 at 07:16

7 Answers7

20

Have you considered using Activator (this is just another option).

T homepageMgmtCarouselItem = Activator.CreateInstance(typeof(T), pageData) as T;
Quintin Robinson
  • 81,193
  • 14
  • 123
  • 132
  • 1
    yes, I have considered it. I read this article. http://www.dalun.com/blogs/05.27.2007.htm But I'd prefer not to go that way if avoidable. I like the syntax suggested in the other question But thanks all the same for the suggestion – ChrisCa Nov 05 '09 at 17:52
18

Jared's answer is still a good way to go - you just need to make the constructor take the Func<PageData, T> and stash it for later:

public class HomepageCarousel<T> : List<T> where T: IHomepageCarouselItem
{
    private readonly Func<PageData, T> factory;

    public HomepageCarousel(Func<PageData, T> factory)
    {
        this.factory = factory;
    }

    private List<T> GetInitialCarouselData()
    {
       List<T> carouselItems = new List<T>();

       if (jewellerHomepages != null)
       {
            foreach (PageData pageData in jewellerHomepages)
            {
                T homepageMgmtCarouselItem = factory(pageData);
                carouselItems.Add(homepageMgmtCarouselItem);
            }
       }
       return carouselItems;
    }

Then you just pass the function into the constructor where you create the new instance of the HomepageCarousel<T>.

(I'd recommend composition instead of inheritance, btw... deriving from List<T> is almost always the wrong way to go.)

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 1
    I've never liked this way of doing it, and usually default to the Activator technique (as Quintin suggested) unless use of reflection is going to have an unacceptible perfomance impact. – philsquared Nov 05 '09 at 17:50
  • thanks Tony. Can you elaborate on how you would use composition rather than inheritance I initially had a property called CarouselItems that contained the data. But then changed the class to inherit from List and made the data availiable that way.
    I guess you are saying both ways are not great??
    – ChrisCa Nov 05 '09 at 17:56
  • 3
    It's not the performance impact I mind - it's the "not finding out things are broken until execution time" aspect. – Jon Skeet Nov 05 '09 at 17:56
  • 1
    @Christo Fur: Write a class which *has* a `List` instead of *deriving* from `List`. That way you're in better control over what happens to the data, aside from anything else. – Jon Skeet Nov 05 '09 at 17:57
  • 1
    re "not finding out things are broken until execution time" - agreed. This is where I really miss C++ templates (no, really) – philsquared Nov 05 '09 at 17:59
  • @Jon "not finding out things are broken until execution time" -- Agreed! – Quintin Robinson Nov 05 '09 at 18:07
  • "thanks Tony" - I laugh every time I see something like this :D – vgru Nov 06 '09 at 09:40
  • @Tony - I've added 2 possible solutions to the post - would love some feedback on the merits of each - thanks a lot – ChrisCa Nov 06 '09 at 12:20
  • 1
    I don't like the idea of an item having to be a factory for itself - it means creating a "useless" intermediate item. If you want to use a factory-based approach instead of a delegate, that's fine - but write a separate factory interface. Note that you don't *have* to use lambdas to create delegates - you could write a static method in each concrete class, and then just use `new HomepageCarousel< JewellerHomepageCarouselItem>(JewellerHomepageCarouselItem.Create)` – Jon Skeet Nov 06 '09 at 13:58
  • yes, that's what the second option I was trying to describe was A method in each concrete class which is resposible for returning an instance of itself Not sure if we are talking about the same thing or not? – ChrisCa Nov 06 '09 at 14:12
  • @Christo Fur: No, we're not talking about quite the same options, as I'm describing a *static* method - which you'd effectively pass in via a delegate as before. It's just replacing the lambda expression with a method group. – Jon Skeet Nov 06 '09 at 14:28
5

Just to add to other answers:

What you are doing here is basically called projection. You have a List of one type and want to project each item (using a delegate) to a different item type.

So, a general sequence of operations is actually (using LINQ):

// get the initial list
List<PageData> pageDataList = GetJewellerHomepages();

// project each item using a delegate
List<IHomepageCarouselItem> carouselList =
       pageDataList.Select(t => new ConcreteCarousel(t));

Or, if you are using .Net 2.0, you might write a helper class like:

public class Project
{
    public static IEnumerable<Tdest> From<Tsource, Tdest>
        (IEnumerable<Tsource> source, Func<Tsource, Tdest> projection)
    {
        foreach (Tsource item in source)
            yield return projection(item);
    }
}

and then use it like:

// get the initial list
List<PageData> pageDataList = GetJewellerHomepages();

// project each item using a delegate
List<IHomepageCarouselItem> carouselList =
       Project.From(pageDataList, 
           delegate (PageData t) { return new ConcreteCarousel(t); });

I'm not sure how the rest of the code looks like, but I believe that GetInitialCarouselData is not the right place to handle the initialization, especially since it's basically duplicating the projection functionality (which is pretty generic and can be extracted in a separate class, like Project).

[Edit] Think about the following:

I believe right now your class has a constructor like this:

public class HomepageCarousel<T> : List<T>
    where T: IHomepageCarouselItem, new()
{
    private readonly List<PageData> jewellerHomepages;
    public class HomepageCarousel(List<PageData> jewellerHomepages)
    {
        this.jewellerHomepages = jewellerHomepages;
        this.AddRange(GetInitialCarouselData());
    }

    // ...
}

I presume this is the case, because you are accessing a jewellerHomepages field in your method (so I guess you are storing it in ctor).

There are several problems with this approach.

  • You have a reference to jewellerHomepages which is unneccesary. Your list is a list of IHomepageCarouselItems, so users can simply call the Clear() method and fill it with anything they like. Then you end up with a reference to something you are not using.

  • You could fix that by simply removing the field:

    public class HomepageCarousel(List<PageData> jewellerHomepages)
    {
        // do not save the reference to jewellerHomepages
        this.AddRange(GetInitialCarouselData(jewellerHomepages));
    }
    

    But what happens if you realize that you might want to initialize it using some other class, different from PageData? Right now, you are creating the list like this:

    HomepageCarousel<ConcreteCarousel> list =
         new HomepageCarousel<ConcreteCarousel>(listOfPageData);
    

    Are you leaving yourself any option to instantiate it with anything else one day? Even if you add a new constuctor, your GetInitialCarouselData method is still too specific to use only PageData as a source.

Conclusion is: Do not use a specific type in your constructor if there is no need for it. Create actual list items (concrete instances) somewhere else.

vgru
  • 49,838
  • 16
  • 120
  • 201
  • thanks for the suggestion - see edit post. What do you think? – ChrisCa Nov 06 '09 at 12:22
  • I think that `HomepageCarousel` class knows too much about the rest of the world (violating Single Responsibility Principle). If you have some functionality attached to the **IHomepageCarouselItem** interface, then you should handle **only that** functionality in your class. Actual implementation should be delegated to the class caller (as shown in Jon's answer), or you could simply create appropriate instances somewhere else. After all, this is simply a List of items (probably with some IHomepageCarouselItem-related functionality). Why would `List` ever need to create instances of `T`? – vgru Nov 06 '09 at 12:53
  • It needs to create instances of T in order populate the list i.e. to make the List have some data in – ChrisCa Nov 06 '09 at 14:20
  • What I am trying to say is - it shouldn't be List's responsibility to populate itself. – vgru Nov 06 '09 at 14:26
  • Well I took Jon's suggestion and changed the class so it doesn't inherit from List anymore. It just Has-A List as a property. Would you still say the object shouldn't be responsible for populating itself in that case? The object is meant to represent the data that goes in a Flash Carousel, the List is a bunch of CarouselItems, that are made up of Images, logos, text etc – ChrisCa Nov 06 '09 at 14:40
  • thanks a lot for taking the time to add further comments. Its appreciated. JewellerHomepages is initialised in the ctor. But it is not passed in through the constructor. It is populated from a structure in the CMS we use. I have heavily refactored this now so I no longer inherit from List. I expose a an ienumerable as a property called CarouselItems thanks again (up voted your answer) – ChrisCa Nov 10 '09 at 12:16
1

It's a C# and CLR handicap, you cannot pass an argument to new T(), simple.

If you're coming from a C++ background this used be NOT-broken and TRIVIAL. PLUS you don't even require an interface/constraint. Breakage all over the place, and without that functional factory 3.0 hack you are forced to do 2-pass initialisation. Managed Blasphemy!

Do the new T() first and then set the property or pass an exotic initialiser syntax or as all well suggested use the Pony's functional workaround.. All yucky but that's the compiler and runtime idea of 'generics' for you.

rama-jka toti
  • 1,404
  • 10
  • 16
  • 1
    The point of generics are to be generic. Making an implementation with specific requirements to type implementation such as expecting a constructor with certain arguments not generic and hence not allowed for generics. That rule ensures that generics are actually generic – Rune FS Nov 06 '09 at 13:12
  • 1
    @Rune FS: Then why can you place other restraints on them like base classes or class/structness? – RCIX Nov 07 '09 at 01:58
  • 1
    @rama-jka toti: finally somebody called the spade a spade. Anyone coming from the C++ background is just put back by this. – andriej Aug 23 '11 at 20:57
0

There is another solution possible, rather dirty one.

Make IHomepageCarouselItem have "Construct" method which takes pageData as parameter and returns IHomepageCarouselItem.

Then do this:

   T factoryDummy = new T();
   List<T> carouselItems = new List<T>();

   if (jewellerHomepages != null)
   {
        foreach (PageData pageData in jewellerHomepages)
        {
            T homepageMgmtCarouselItem = (T)factoryDummy.Construct(pageData);
            carouselItems.Add(homepageMgmtCarouselItem);
        }
   }
   return carouselItems;
yk4ever
  • 791
  • 5
  • 7
0

I'd Probably go for the suggestion from Tony "jon" the Skeet pony but there's another way of doing it. So mostly for the fun here's a different solution (that have the down side of failing at runtime if you forget to implement the needed method but the upside of not having to supply a factory method, the compiler will magically hook you up.

public class HomepageCarousel<T> : List<T> where T: IHomepageCarouselItem
{

    private List<T> GetInitialCarouselData()
    {
       List<T> carouselItems = new List<T>();

       if (jewellerHomepages != null)
       {
            foreach (PageData pageData in jewellerHomepages)
            {
                T homepageMgmtCarouselItem = null;
                homepageMgmtCarouselItem = homepageMgmtCarouselItem.create(pageData);
                carouselItems.Add(homepageMgmtCarouselItem);
            }
       }
       return carouselItems;
    }
}

public static class Factory
{
   someT create(this someT, PageData pageData)
   {
      //implement one for each needed type
   }

   object create(this IHomepageCarouselItem obj, PageData pageData)
   {
      //needed to silence the compiler
      throw new NotImplementedException();
   }
}

just to repeat my "disclaimer" this is very much ment to serve as a reminder that there can be rather different approaches to solving the same problem they all have draw backs and strengths. One draw back of this approach is that it's part black magic ;)

T homepageMgmtCarouselItem = null;
homepageMgmtCarouselItem = homepageMgmtCarouselItem.create(pageData);

but you avoid the perculiar constructor taking a delegate argument. (but I usually go for that approach unless I was using a dependency injection mechanism to supply the factory class for me. Which insidentally is the kind of DI framework I'm working on in my sparetime ;p)

Rune FS
  • 21,497
  • 7
  • 62
  • 96
  • If I got this right, this would require writing a huge if/else/else/else... block inside the Create extension method to handle each type properly? What's the point in using generics then? – vgru Nov 06 '09 at 09:26
  • I've tried something similar - see edit post. What do you think? – ChrisCa Nov 06 '09 at 12:21
  • @Groo you don't need a huge if-else. You'd need a specific extension method for each type. – Rune FS Nov 06 '09 at 13:07
0

Why don't you just put a static "constructor" method on the interface? A little hacky I know, but you gotta do what you gotta do...

RCIX
  • 38,647
  • 50
  • 150
  • 207