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 my class hierarchy looks like this:

  • P
  • I : P
  • O : P

I have this method. The goal of this method is to return a new object of type I or O depending on a certain condition.

 public P GetP()
 {
    this.Session.Begin();

    var pDetail = GetPDetail();

    if (pDetail == null)
    {
        this.Session.Close();
        return null;
    }            

    switch (pDetail.Code)
    {
        case "1":                    
            var i = new I();
            i.PDetail = pDetail;
            FillBaseP(i);
            FillI(i);
            this.Session.Close();
            return i;
        case "3":
            var o = new O();
            o.PDetail = pDetail;
            FillBaseP(o);
            FillO(o);
            this.Session.Close();
            return o;
        default:
            throw new Exception("Code is invalid");
     }
 }

I'm looking to rewrite this in a more logical way. Both case "1" and case "3" are doing some of the same work.

If I'm constantly checking type like If(var is O) or If(var is I) I feel as though I'm doing something wrong. Is this the case?

share|improve this question
1  
Is this something that could fall under a factory pattern? Change the factory based on the value of pDetail.Code, then call a standard "create" method? (I'm not sure how extensible you need this to be). –  Brad Christie Feb 5 '13 at 16:12
    
Additionally you may want to consider dealing in terms of an interface, especially since much of the code under the case "1" and "3" look very similar. –  Ryan Gates Feb 5 '13 at 16:40
    
@RyanGates I think that's the question Chance is asking: How to do that? –  svick Feb 5 '13 at 20:44
add comment

3 Answers

  1. I think it would be better if Session.Begin() returned an IDisposable, which would call Session.Close() in its Dispose(). That way, you can enclose the whole method inside using and you don't have to worry about calling it properly. (For example, it's not called in your code if the code is invalid.)

  2. You can extract the creation of I or O into a generic method. It will also have a delegate parameter that represents the type-specific fill method.

  3. It's better if you throw a specific exception (maybe something like InvalidCodeException), not Exception.

With these changes, the code would look like this:

private P CreateP<T>(PDetail pDetail, Action<T> fillT) where T : P, new()
{
    var p = new T { PDetail = pDetail };
    FillBaseP(p);
    fillT(p);
    return p;
}

public P GetP()
{
    using (this.Session.Begin())
    {
        var pDetail = GetPDetail();

        if (pDetail == null)
            return null;

        switch (pDetail.Code)
        {
        case "1":
            return CreateP<I>(pDetail, FillI);
        case "3":
            return CreateP<O>(pDetail, FillO);
        default:
            throw new InvalidCodeException("Code is invalid");
        }
    }
}

If I'm constantly checking type like If(var is O) or If(var is I) I feel as though I'm doing something wrong. Is this the case?

Yeah, probably. But it's hard to tell (and advise a better approach) without seeing the code.

share|improve this answer
add comment

If I'm constantly checking type like If(var is O) or If(var is I) I feel as though I'm doing something wrong. Is this the case?

Well, your code does not fully utilize the OOP paradigm, which is why you keep checking types manually instead of delegating that to the framework/language.

Consider the following code:

class P
{
    protected P(PDetail detail)
    {
        this.Fill(detail);
    }

    protected virtual void Fill(PDetail detail)
    {
        // FillBaseP() imp.
    }
}

class I : P
{
    public I(PDetail detail)
        : base(detail) 
    {
        // some imp.
    }

    protected override void Fill(PDetail detail)
    {
        base.Fill(detail);
        // FillI() imp.
    }
}

class O : P
{
    public O(PDetail detail)
        : base(detail) { }

    protected override void Fill(PDetail detail)
    {
        base.Fill(detail);
        // FillO() imp.
    }
}

    public P GetP()
    {
        using (this.Session.Begin())
        {
            var pDetail = GetPDetail();

            if (pDetail == null)
                return null;

            switch (pDetail.Code)
            {
                case "1":
                    return new I(pDetail);
                case "3":
                    return new O(pDetail);
                default:
                    throw new InvalidCodeException("Code is invalid");
            }
        }
    }

Now each subclass contains its own implementation details, and when the base contractor calls Fill(), the most recent implementation is called (for O, O.Fill, for I, I.Fill). Then, the implementation calls the base Fill (your old FillBaseP), and than performs it's own magic.

new I(d) call chain:

I(d)
 P(d)
  I.Fill(d)
   P.Fill(d)
  back to I.Fill(d) (to // FillI() imp.)
 back to P(d) 
back to I(d) (to // some imp.)

Note that aside from the construction part, the code never checks the type or the details variable. It just assumes that each subclass override Fill() correctly.

Hope that helps!

share|improve this answer
    
My issue is I and O have properties and methods that are not common, so they are not in P and need to be accessed in the code. This leaves me having to downcast P to the correct concrete type. I'm not certain any way to get around the downcasting. Should I not be using base type P then? –  Chace Fields Feb 9 '13 at 15:00
    
You should decide what is the 'interface' of I&O (i.e, what they have in common, like 'Fill') and define it in P, and what is specific to each of them (i.e, the underlying properties). I would then try to move as much of the type-dependent code into the classes. For example, say you need to Print() them, and I is a circle and O is square. You need I's Radius and O's SideLength, but both can provide the print interface. –  Ohad Feb 9 '13 at 22:54
add comment

I'd use a combination of the abstract factory, template method and null object patterns. In order to satisfy the single responsibility principle, you would need to use the abstract factory pattern to create a specialized factory to fill the object.

I have made some assumptions because very little detail has been provided.

internal class Program
{
    private static void Main(string[] args) {
        //Example usage
        var detail = new PDetail();
        var instanceFactory = new PFactory();
        var instance = instanceFactory.CreateFrom(detail);
    }
}

public abstract class P {}

public class NullP : P {}

public class I : P {}

public class O : P {}

public class PDetail
{
    public int Code { get; set; }
}

public class PFactory
{
    private static readonly IDictionary<int, Func<IPFactory>> factories;

    static PFactory() {
        //factories is a map between the code and a method that returns an IPFactory
        factories = new Dictionary<int, Func<IPFactory>> {
            { 1, () => new IFactory() }, 
            { 3, () => new OFactory() }
        };
    }

    private static IPFactory CreateFactory(int code) {
        //This abstract factory creates a specific instance factory that will create and fill an instance of P
        if (!factories.ContainsKey(code)) {
            return new NullPFactory();
        }

        var createFactory = factories[code];
        return createFactory();
    }

    public P CreateFrom(PDetail detail) {
        //1. Create the specific factory required to produce the specific instance based on a discriminator (detail.Code)
        var factory = CreateFactory(detail.Code);
        //2. Create and fill the instance and return it
        return factory.CreateAndFill(detail);
    }
}

public interface IPFactory
{
    P CreateAndFill(PDetail detail);
}

public abstract class PFactoryBase<T> : IPFactory
    where T : P
{
    public P CreateAndFill(PDetail detail) {
        //Template method pattern -> Fill is implemented by concrete factories ex. IFactory, OFactory
        var instance = CreateInstance();
        FillBase(instance, detail);
        Fill(instance, detail);
        return instance;
    }

    private void FillBase(P instance, PDetail detail) {
        //Fill base
    }

    protected abstract T CreateInstance();

    protected abstract void Fill(T instance, PDetail detail);
}

public class NullPFactory : PFactoryBase<NullP>
{
    protected override NullP CreateInstance() {
        return new NullP();
    }

    protected override void Fill(NullP instance, PDetail detail) {
        //Do nothing?
    }
}

public class OFactory : PFactoryBase<O>
{
    protected override O CreateInstance() {
        return new O();
    }

    protected override void Fill(O instance, PDetail detail) {
        //Fill the instance
    }
}

public class IFactory : PFactoryBase<I>
{
    protected override I CreateInstance() {
        return new I();
    }

    protected override void Fill(I instance, PDetail detail) {
        //Fill the instance
    }
}
share|improve this answer
1  
I really don't think the null object pattern is appropriate here. If you can't create the proper object, throw an exception, don't create something that looks like it's what you wanted, but actually isn't. –  svick Feb 10 '13 at 18:42
    
I agree with you in the case that P and O are anemic domain models or data objects. For components I would stick to a null object pattern (ex. pricing strategy) to ensure consistent usage. Unfortunately the question posed is vague in these matters - it simply attains that different objects are created based on a 'Code' discriminator - no usage examples are given. I tend to use exceptions only in exceptional circumstances (when the fault lays with the developer or external libraries) and never for control flow unless no other options are available (ex. Amazon SDK sending e-mail via SES). –  Shelakel Feb 10 '13 at 19:29
add 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.