Refactoring implementation in.NET

Question

I have implemented the following code:

class A
{
    abstract void f1(Object obj, Object data);
}

class A1:A
{
    void f1(Object obj, Object data)
    {
        m1(obj,data);
    }

    void m1(Object obj, Object data)
    {
    }
}

class A2:A
{
    void f1(Object obj, Object data)
    {
        m2(obj, data);
    }
    void m2(Object obj, Object data)
    {
    }
}

static class Factory
{
    public static A GetInstance(int i)
    {
        if (i == 1)
            return new A1();
        else if (i == 2)
            return new A2();
        else
            return null;
    }
}

Methods are being called from some other part of the application, as following:

var a = Factory.GetInstance(1);
a.f1(obj, data);

In the above code, the abstract method f1() has two parameters obj and data. While I reviewed the code, I found the parameter data can be determined from the first parameter obj itself. The area of the application from where the method f1() is being called, there the Libraries for getting data are not available. So I refactored the code further as following in order to send one parameter obj. Following is the implementation:

class A
{
    public Object data { get; set; }
    abstract void f1(Object obj);
}

class A1:A
{
    void f1(Object obj)
    {
        m1(obj,data);
    }

    void m1(Object obj, Object data)
    {
    }
}

class A2:A
{
    void f1(Object obj)
    {
        m2(obj, data);
    }
    void m2(Object obj, Object data)
    {
    }
}

static class Factory
{
    public static A GetInstance(int i)
    {
        if (i == 1)
            return new A1();
        else if (i == 2)
            return new A2();
        else
            return null;
    }
}

Now, I am calling the method as following:

var a = Factory.GetInstance(1);
a.data = GetData();
a.f1(obj);

Please let me know whether or not the refactoring is correct. Also, please suggest some ways for better refactoring.

Answer 1

The code still looks a bit strange to me.

obj.f1(obj);

Why are you passing the object as a parameter when the method already is a member? Skip the parameter and use this instead.

You should also try to minimize the number of method the user needs to call to perform one thing. Requiring obj.data to be set before calling obj.f1 is error prone. It would be better to let the obj constructor call GetData to extract the data from the obj.

Answer 2

In order to do a proper refactoring, you should move your classes somewhere, where you can calculate data. If you cannot do so, then you'll be better off with your original version. The second version is completely counterintuitive. Without digging into implementation details it is impossible to guess, that you need to set data property for f1 method to work.

Also returning null from factory (unless it is a valid value) is bad code style. You should throw NotSupportedException instead