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.
