Refactoring three property methods

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};
        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 };
        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 };
        //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.

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

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

Notice the method is returning an IEnumerable, not a List - 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.

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

StackExchange Code Review Q#56583, answer score: 12