8
\$\begingroup\$

I am trying to refactor these three methods into one:

    public StatusReturn UpdateProperty(int id, int onlineReportId)
    {
        //Yes I realize I should not be using ref variables but the queryPermissions is an external dll.
        var webQueryInputProperty = new WebQueryInputProperty();
        //This line is different
        _queryPermissions.GetInputPropertyByKey(id, ref webQueryInputProperty);
        webQueryInputProperty.WQI_Key = onlineReportId;
        webQueryInputProperty.WQIP_Key = id;
        var webQueryInputProperties = new List<WebQueryInputProperty>{webQueryInputProperty};
        return _queryPermissions.UpdateWQInputProperties(webQueryInputProperties);
    }

    public StatusReturn AddProperty(int id, int onlineReportId)
    {
       var webQueryInputProperty = new WebQueryInputProperty();
        webQueryInputProperty.WQIP_Key = id;
        webQueryInputProperty.WQI_Key = onlineReportId;
        var webQueryInputProperties = new List<WebQueryInputProperty> { webQueryInputProperty };
        return _queryPermissions.UpdateWQInputProperties(webQueryInputProperties);
    }

    public StatusReturn DeleteProperty(int id, int onlineReportId)
    {
        var webQueryInputProperty = new WebQueryInputProperty();
        _QueryPermissions.GetInputPropertyByKey(id, ref webQueryInputProperty);
        webQueryInputProperty.WQI_Key = onlineReportId;
        webQueryInputProperty.WQIP_Key = id;
        var webQueryInputProperties = new List<WebQueryInputProperty> { webQueryInputProperty };
        //this line is different
        return _QueryPermissions.DeleteWebQueryInputProperties(webQueryInputProperties);
    }

I want to refactor the methods, remove the one line from AddProperty and change the return statement. I would rather not copy and paste.

\$\endgroup\$
  • 3
    \$\begingroup\$ Please do not recreate the [refactoring] tag. We have had it removed because Code Review questions already involve refactoring. \$\endgroup\$ – Jamal Jul 9 '14 at 19:09
12
\$\begingroup\$

I am trying to refactor these three methods into one

That would be, uh, de-factoring. I don't know of any merge methods refactoring - when people refactor code, they tend to extract functionality so as to have more methods (or classes and/or interfaces) that are also more simple, which means they do very little.

Taking 3 methods that do 3 different (albeit similar) things, and merging into one, would be a very bad idea. You have redundant code though, and that can be refactored.


Refactoring 1: Extract Method

All 3 methods have a dependency in common:

var webQueryInputProperty = new WebQueryInputProperty();

...and they all do the same thing with it (that would be even more obvious if the two properties were set in the same order in all 3 methods):

webQueryInputProperty.WQIP_Key = id;
webQueryInputProperty.WQI_Key = onlineReportId;
var webQueryInputProperties = new List<WebQueryInputProperty>{webQueryInputProperty};

There's this instruction, too, that the AddProperty method isn't calling:

_queryPermissions.GetInputPropertyByKey(id, ref webQueryInputProperty);

This means you could have a 4th, private method like this, whose job is to provide the other 3 methods with a list that contains a webQueryInputProperty object:

private IEnumerable<WebQueryInputProperty> GetWebQueryInputProperties(int id, int onlineReportId, bool isNewItem = false)
{
    var webQueryInputProperty = new WebQueryInputProperty();
    if (!isNewItem)
    {
        _queryPermissions.GetInputPropertyByKey(id, ref webQueryInputProperty);
    }

    webQueryInputProperty.WQIP_Key = id;
    webQueryInputProperty.WQI_Key = onlineReportId;

    return new List<WebQueryInputProperty> { webQueryInputProperty };
}

Notice the method is returning an IEnumerable<T>, not a List<T> - you could just as well return an inline array, like this:

    return new[] { webQueryInputProperty };

That is, if the 3rd-party library methods you're passing it to don't actually require a List<T>.

At this point you have this already:

public StatusReturn UpdateProperty(int id, int onlineReportId)
{
    var properties = GetWebQueryInputProperties(id, onlineReportId);
    return _queryPermissions.UpdateWQInputProperties(properties);
}

public StatusReturn AddProperty(int id, int onlineReportId)
{
    var properties = GetWebQueryInputProperties(id, onlineReportId, true);
    return _queryPermissions.UpdateWQInputProperties(properties);
}

public StatusReturn DeleteProperty(int id, int onlineReportId)
{
    var properties = GetWebQueryInputProperties(id, onlineReportId);
    return _queryPermissions.DeleteWQInputProperties(properties);
}

That's nice, but the bool parameter is sticking out like a sore thumb. It's begging for an abstraction:

private enum DataOperation
{
    Update,
    Add,
    Delete
}

And then:

private IEnumerable<WebQueryInputProperty> GetWebQueryInputProperties(int id, int onlineReportId, DataOperation operation)
{
    var webQueryInputProperty = new WebQueryInputProperty();
    if (operation == DataOperation.Add)
    {
        _queryPermissions.GetInputPropertyByKey(id, ref webQueryInputProperty);
    }

    ...

Which turns the call sites into this:

public StatusReturn UpdateProperty(int id, int onlineReportId)
{
    var properties = GetWebQueryInputProperties(id, onlineReportId, DataOperation.Update);
    return _queryPermissions.UpdateWQInputProperties(properties);
}

public StatusReturn AddProperty(int id, int onlineReportId)
{
    var properties = GetWebQueryInputProperties(id, onlineReportId, DataOperation.Add);
    return _queryPermissions.UpdateWQInputProperties(properties);
}

public StatusReturn DeleteProperty(int id, int onlineReportId)
{
    var properties = GetWebQueryInputProperties(id, onlineReportId, DataOperation.Delete);
    return _queryPermissions.DeleteWQInputProperties(properties);
}

Refactoring 2: Rename

WQI_Key and WQIP_Key are bad names. Don't use underscores in the middle of an identifier, and don't use abbreviations. If WQI_Key is supposed to be a report ID, why not call it ReportId?

If the class this code is written is is cohesive, then you can drop the Property suffix (the type name says Property too, right?) and have Add, Update and Delete methods instead of AddProperty, UpdateProperty and DeleteProperty.

If it's not cohesive, then you have an AddXxxx scheme and you probably have a whole other class to extract.

you're way too verbose here:

var webQueryInputProperty = new WebQueryInputProperty();

I would have had this:

var property = new WebQueryInputProperty();

I suspect _QueryPermissions is a typo; the leading underscore makes me believe it's referring to a private or private readonly field (in which case the correct naming is with the lowercase q).


Lastly, StatusReturn is a bad name for a type - literally anything with a Return suffix is a bad name for a type.

Actually, it's probably a bad idea to even have to deal with that type in the first place, let alone to spread it into your own code.

Instead of leaving it to the client code to verify the status and - I bet - make sure no error occurred and that the method call was successful, you could have a private static method to analyze a StatusReturn, and throw an exception in case of a non-successful method call.

Assuming ReturnResult is an enum (and that you create custom exception types as needed), it could look something like this (don't default to success!):

private static void ValidateResultOrThrow(ReturnResult result)
{
    switch (result)
    {
        case ReturnResult.Success:
            return;

        //case ReturnResult.ConnectionError:
        //    throw new NoNetworkException("Unable to connect.");

        //case ReturnResult.InvalidIdError:
        //    throw new ArgumentException("Specified id doesn't exist.");

        case default:
            throw new UnknownResultException("Something bad happened.");
    }
}

And then your methods could return void, and either succeed or throw - which is probably what the client code would be expecting - and you keep the ReturnResult ugliness nicely abstracted away:

public void Update(int id, int reportId)
{
    var properties = GetWebQueryInputProperties(id, reportId, DataOperation.Update);
    var result = _queryPermissions.UpdateWQInputProperties(properties);
    ValidateResultOrThrow(result);
}

public void Add(int id, int reportId)
{
    var properties = GetWebQueryInputProperties(id, reportId, DataOperation.Add);
    var result = _queryPermissions.UpdateWQInputProperties(properties);
    ValidateResultOrThrow(result);
}

public void Delete(int id, int reportId)
{
    var properties = GetWebQueryInputProperties(id, reportId, DataOperation.Delete);
    var result = _queryPermissions.DeleteWQInputProperties(properties);
    ValidateResultOrThrow(result);
}

Hmm.. Perhaps the GetWebQueryInputProperties calls would read better if the DataOperation parameter was first. A reorder parameters (or change signature) refactoring would fix that.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Yes the upperCase q was a typo. I missed it when sanitizing my code. I hate having the return value of statusReturn but it is given to me from the _queryPermissions calls and honestly I convert it to a more usable field one level up from the service. I cannot change the variable names you mentioned because those objects exist in the 3rd party dll's I am just assigning them now so that when I call automapper it has the proper data to map. Besides that, this is excellent and gives me a lot too look into. \$\endgroup\$ – Robert Jul 10 '14 at 12:07
  • 1
    \$\begingroup\$ I wouldn't let ReturnResult anywhere further than the service; your service calls should either succeed, or fail with a meaningful exception, leaving your calling code blissfully unaware of that type.. just my two cents ;) \$\endgroup\$ – Mathieu Guindon Jul 10 '14 at 18:06
  • \$\begingroup\$ I think this still has too much repetition. You could use a delegate to make the methods into one-liners, something like PerformWebQuery(is, reportId, DataOperation.Add, _queryPermissions.UpdateWQInputProperties). Or maybe make the extracted method choose the _queryPermissions method based on the DataOperation parameter by itself. \$\endgroup\$ – svick Jul 13 '14 at 12:56
  • \$\begingroup\$ @svick could you give an example? \$\endgroup\$ – Robert Jul 14 '14 at 12:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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