Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm building a generic flat file reader which looks something like this.

 public class GenericReader<TComposite, THeader, TData, TTrailer> 
    where TComposite : GenericComposite<THeader, TData, TTrailer>, new()
    where THeader : new()
    where TData : new()
    where TTrailer : new()
{
    public TComposite Read()
    { 
        // read stuff, do parsing etc
        var composite = new TComposite();

        composite.Header = new THeader();
        composite.Data = new TData();
        composite.Trailer = new TTrailer();

        return composite;
    }        
}

It could be consumed like so.

var reader = new GenericReader<Composite<Header, Data, Trailer>, Header, Data, Trailer> ();

var composite = reader.Read();
Console.WriteLine(composite.Data.SomeProperty);

Console.ReadLine();

Here are the classes used.

public class Composite<THeader, TData, TTrailer> : GenericComposite<THeader, TData, TTrailer>
{

}

public class GenericComposite<THeader, TData, TTrailer>
{
    public THeader Header { get; set; }

    public TData Data { get; set; }

    public TTrailer Trailer { get; set; }
}

public class Header {
    public string SomeProperty { get { return "SomeProperty"; } } 
}

public class Data {
    public string SomeProperty { get { return "SomeProperty"; } } 
}

public class Trailer {
    public string SomeProperty { get { return "SomeProperty"; } } 
}

Is there a way how I could remove or encapsulate that generic type information in the GenericReader? I'm looking for an extra pair of eyes to show me something what I've been missing. We already did something with returning interfaces, and making the consumer do a cast, but that just moves the responsibility to the wrong location in my opinion, plus there is a small performance penalty.

Thanks.

share|improve this question
1  
What is the thinking behind the Composite and GenericComposite classes? They seem the same to me. –  Jeff Vanzella Nov 27 '12 at 19:05
    
What's the intent of the Composite class in the first place? Why is that separate from the GenericReader? –  Bobson Nov 27 '12 at 19:54
    
Composite class composites header, data and trailer, makes for one thing to return. You can see it as a logical file. –  JefClaes Nov 27 '12 at 19:57
    
@JeffVanzella You're absolutely right, it's not necessary. Now I can remove that first type parameter. –  JefClaes Nov 27 '12 at 20:39
add comment

3 Answers

Having so many new operations -- seems like you'd want to use some kind of a factory, injected in the constructor, or the instances themselves injected in the constructor, instead of constructing the classes inside the constructor.

share|improve this answer
    
The real imp uses a factory to create the instances. I'm doing more than composing my composite. –  JefClaes Nov 27 '12 at 19:50
add comment

Why not simply replace

public class GenericReader<TComposite, THeader, TData, TTrailer> 
    where TComposite : GenericComposite<THeader, TData, TTrailer>, new()
    where THeader : new()
    where TData : new()
    where TTrailer : new()
{
    public TComposite Read()
    {
        // read stuff, do parsing etc 
        var composite = new TComposite();

        composite.Header = new THeader();
        composite.Data = new TData();
        composite.Trailer = new TTrailer();

        return composite;
    }        
}

with

public class GenericReader<THeader, TData, TTrailer> 
    where THeader : new()
    where TData : new()
    where TTrailer : new()
{
    public GenericComposite<THeader, TData, TTrailer> Read()
    {
        // read stuff, do parsing etc 
        var composite = new GenericComposite<THeader, TData, TTrailer>();

        composite.Header = new THeader();
        composite.Data = new TData();
        composite.Trailer = new TTrailer();

        return new composite;
    }        
}

?

share|improve this answer
    
Because I didn't see it. How can I look over that? Thank you, changing that right now. –  JefClaes Nov 27 '12 at 20:32
1  
Sometimes, it just takes another pair of eyes. I've certainly been there, and that's exactly what this SE site is for! Glad to help. –  Bobson Nov 27 '12 at 22:30
add comment

I think the easiest way would be to create a factory type class. The create would take the 3 generics as generics, and the expected return type as a parameter. Then use reflection to create the instance. Before you say anything about using reflection, by creating generics in your function, you are inadvertently using reflection.

public class CompsiteFactory
{
    public GenericComposite<THeader, TData, TTrailer> Create<THeader, TData, TTrailer>(Type compositeType)
        where THeader : new()
        where TData : new()
        where TTrailer : new()
    {
        var finalType =
            compositeType.MakeGenericType(new[] {typeof (THeader), typeof (TData), typeof (TTrailer)});

        var compositeInstance =
            (GenericComposite<THeader, TData, TTrailer>)
            Activator.CreateInstance(finalType);

        compositeInstance.Header = new THeader();
        compositeInstance.Data = new TData();
        compositeInstance.Trailer = new TTrailer();

        return compositeInstance;
    }
}

Modify your GenericReader

public class GenericReader<THeader, TData, TTrailer>
    where THeader : new()
    where TData : new()
    where TTrailer : new()
{
    private readonly CompsiteFactory _factory;

    public GenericReader(CompsiteFactory factory)
    {
        _factory = factory;
    }

    public GenericComposite<THeader, TData, TTrailer> Read(Type compositeType)
    {
        return _factory.Create<THeader, TData, TTrailer>(compositeType);
    }
}

var factory = new CompsiteFactory();
var genericReader = new GenericReader<Header, Data, Trailer>(factory);

var composite = genericReader.GenericComposite(typeof(Composite<,,>));

And you have your instance of GenericComposite.

I know that is not a true factory class, but it will work for this instance. If you want to do it properly, there is a lot of examples on best practices online.

share|improve this answer
    
I'm doing more than composing my instance. The real imp uses something that creates the record instances. –  JefClaes Nov 27 '12 at 19:52
    
@JefClaes. If your doing more than what you are asking for in your review, perhaps post the real impl so reviewers have the real question to work off.... –  dreza Nov 27 '12 at 19:59
    
@dreza - I think that's what the improperly formatted comment is supposed to indicate is happening in there. –  Bobson Nov 27 '12 at 20:02
    
What do you do with the record instances? –  Jeff Vanzella Nov 27 '12 at 20:15
    
@dreza I'm sorry, I put that in there from my mobile phone. Couldn't get it right. –  JefClaes Nov 27 '12 at 20:30
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.