WCF service implementation

I have WCF Service using .NET 4.0.

I have similar methods, similar code and I want use good patterns for improvement the code for get high level quality.
I want reuse code, maybe using Action, and improve exception handling, ...

Is it possible?

Interface

[FaultContract(typeof(MyCompany.VSIntegration.SvcDespliegue.FaultContracts.DefaultFaultContract))]
    [OperationContract]
    QueryLogRes QueryLogsAdvanced(QueryLogReq req);

[FaultContract(typeof(MyCompany.VSIntegration.SvcDespliegue.FaultContracts.DefaultFaultContract))]
[OperationContract]
QueryLogRes QueryLogsByPeticionEV(QueryLogsByPeticionEVReq req);

Implementation

```
public QueryLogRes QueryLogsAdvanced(QueryLogReq req)
{
try
{
IList listLogs;
bool result;

ArgumentHelper.AssertNotNull(req, "request");

Log.Trace(LogCategoryNames.DebugGeneral, "QueryLogsAdvanced(" + req.ToString() + ")");

using (var rpy = new LogsRepository())
{
listLogs = rpy.QueryLogs(req.Proyecto, req.Fecha, req.Servidor, req.Entorno, req.Usuario, req.Maquina, req.result, req.Etiqueta, req.PeticionEV);
}

result = (listLogs != null && listLogs.Count > 0);

Trace.WriteLine("QueryLogsAdvanced Res: " + result);

var res = new QueryLogRes(result, result ? "" : "No se han encontrado registros");
if (!result) return res;

Trace.WriteLine("QueryLogsAdvanced listLogs: " + listLogs.Count);

foreach (var row in listLogs)
{
var dl = new DetailLog();
dl.Entorno = row.Entorno;
dl.Fecha = row.Fecha;
dl.Log = row.Log;
dl.Maquina = row.Maquina;
dl.result = row.result;
dl.Servidor = row.Servidor;
dl.Usuario = row.Usuario;
dl.Etiqueta = row.Etiqueta;
dl.PeticionEV = row.PeticionEV;
res.DetailsLogs.Add(dl);
}

Log.Trace(LogCategoryNames.DebugGeneral, "QueryLogsAdva

Good

• Wrapping IDisposable's within a using statement

• Coding against an interface instead of an implementation

• Most parameters are named well

Bad

• Some parameters aren't named well

• Some usages of var, where the type isn't seen immediatetely

• unneccessary use of IList.Count instead of Any()

• Violates DRY principle

Refactoring

-
As the names req, rpy, row, dl, res, result aren't meaningful to Mr.Maintainer we will just rename them.

req -> request

res -> queryLogResult

result -> hasLogEntries

rpy-> logRepository

row -> logEntry

dl -> detailLog

-
As the using of the var type at the foreach loop is to many rows away from the declaration of the listLogs variable, this is't as readable and understandable for Mr.Maintainer as it could be.

-
As at declaration of an boolean variable the initial value is always false we can either skip the = false or add it for every boolean variable in the code.

-
For cases we don't need to know the exact number of items in a IList, because we need to only know if there is any item in the list, we should use the .Any() method as this is faster.

-
The code after we have queried the repository is, setting aside some strings for logging, identical. This is screaming for extraction to separate method(s).

Let us start now with writing code. First we write a method which takes a LogsDesplieguesDto and returns a DetailLog object.

private DetailLog ToDetailLog(LogsDesplieguesDto logEntry)
{
    DetailLog detailLog = new DetailLog();

    detailLog.Entorno = logEntry.Entorno;
    detailLog.Fecha = logEntry.Fecha;
    detailLog.Log = logEntry.Log;
    detailLog.Maquina = logEntry.Maquina;
    detailLog.result = logEntry.result;
    detailLog.Servidor = logEntry.Servidor;
    detailLog.Usuario = logEntry.Usuario;
    detailLog.Etiqueta = logEntry.Etiqueta;
    detailLog.PeticionEV = logEntry.PeticionEV;

    return detailLog;
}

Now we add a new method which takes a IList and returns a IList object.

private IList ToDetailLogList(IList listLogs)
{
    IList detailLogs = new List();

    foreach (LogsDesplieguesDto logEntry in listLogs)
    {
        detailLogs.Add(ToDetailLog(logEntry));
    }
    return detailLogs;
}

Next we add a method to convert a IList to a QueryLogRes object.

private const String noEntries = "No se han encontrado registros";
private QueryLogRes ToQueryLogResult(IList listLogs)
{
    bool hasLogEntries = (listLogs != null && listLogs.Any());

    var queryLogResult = new QueryLogRes(hasLogEntries, hasLogEntries ? String.Empty : noEntries);

    if (!hasLogEntries) { return queryLogResult; }

    queryLogResult.DetailsLogs.AddRange(ToDetailLogList(listLogs));

    return queryLogResult;
}

Now the two methods of the original code will look like this

public QueryLogRes QueryLogsAdvanced(QueryLogReq request)
{
    try
    {
        ArgumentHelper.AssertNotNull(request, "request");

        IList listLogs;
        using (var logRepository = new LogsRepository())
        {
            listLogs = logRepository.QueryLogs(request.Proyecto, 
                request.Fecha, request.Servidor, 
                request.Entorno, request.Usuario, 
                request.Maquina, request.result, 
                request.Etiqueta, request.PeticionEV);
        }

        return ToQueryLogResult(listLogs);
    }
    catch (Exception exc)
    {
        Exception auxExc = exc;
        bool rethrow = false;
        MyCompany.Frk.Exceptions.ExceptionHandler.HandleExceptionNotRethrowing(exc, NOMBRE_POLITICA_VSINTEGRATION, out auxExc, out rethrow);

        if (rethrow)
        {
            throw new FaultException(new DefaultFaultContract(-1, auxExc.Message, Guid.Empty));
        }

        return new QueryLogRes(false, exc.Message);
    }
}

public QueryLogRes QueryLogsByPeticionEV(QueryLogsByPeticionEVReq request)
{
    try
    {
        ArgumentHelper.AssertNotNull(request, "request");
        ArgumentHelper.AssertNotNullOrEmpty(request.PeticionEV, "PeticionEV");

        IList listLogs;
        using (var logRepository = new LogsRepository())
        {
            listLogs = logRepository.QueryLogsByPeticionEV(request.PeticionEV);
        }

        return ToQueryLogResult(listLogs);
    }
    catch (Exception exc)
    {
        Exception auxExc = exc;
        bool rethrow = false;
        MyCompany.Frk.Exceptions.ExceptionHandler.HandleExceptionNotRethrowing(exc, NOMBRE_POLITICA_VSINTEGRATION, out auxExc, out rethrow);

        if (rethrow)
        {
            throw new FaultException(new DefaultFaultContract(-1, auxExc.Message, Guid.Empty));
        }

        return new QueryLogRes(false, exc.Message);
    }
}

For the exception handling, I don't know where the exception can occur.

Additional thougths

You can also convert the ToQueryLogResult(), ToDetailLogList() and ToDetailLog() methods to extension methods.

About the naming of c

StackExchange Code Review Q#63842, answer score: 4