Need help with my code using a base class?

Question

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?

Answer 1

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.

Answer 2

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!

Answer 3

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
    }
}