Need help with my code using a base class?

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?

-
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.)

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

-
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(PDetail pDetail, Action 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(pDetail, FillI);
        case "3":
            return CreateP(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.

StackExchange Code Review Q#21342, answer score: 5