4
\$\begingroup\$

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<T>, 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<LogsDesplieguesDto> listLogs;
        bool result;

        ArgumentHelper.AssertNotNull<QueryLogReq>(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, "QueryLogsAdvanced. Logs encontrados: " + listLogs.Count);
        Trace.WriteLine("QueryLogsAdvanced. Logs encontrados: " + listLogs.Count);

        return res;
    }
    catch (Exception exc)
    {
        Log.Trace(LogCategoryNames.Errores, "EXCEPCION en 'QueryLogs': " + exc.Message);

        Exception auxExc = exc;
        bool rethrow = false;
        MyCompany.Frk.Exceptions.ExceptionHandler.HandleExceptionNotRethrowing(exc, NOMBRE_POLITICA_VSINTEGRATION, out auxExc, out rethrow);

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

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


public QueryLogRes QueryLogsByPeticionEV(QueryLogsByPeticionEVReq req)
{
    try
    {
        IList<LogsDesplieguesDto> listLogs;
        bool result;

        ArgumentHelper.AssertNotNull<QueryLogsByPeticionEVReq>(req, "request");
        ArgumentHelper.AssertNotNullOrEmpty(req.PeticionEV, "PeticionEV");

        using (var rpy = new LogsRepository())
        {
            listLogs = rpy.QueryLogsByPeticionEV(req.PeticionEV);
        }

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

        QueryLogRes res = new QueryLogRes(result, result ? "" : "No se han encontrado registros");
        if (result)
        {
            foreach (var row in listLogs)
            {
                DetailLog 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);
            }

        }

        return res;
    }
    catch (Exception exc)
    {
        Log.Trace(LogCategoryNames.Errores, "EXCEPCION en 'QueryLogs': " + exc.Message);

        Exception auxExc = exc;
        bool rethrow = false;
        MyCompany.Frk.Exceptions.ExceptionHandler.HandleExceptionNotRethrowing(exc, NOMBRE_POLITICA_VSINTEGRATION, out auxExc, out rethrow);

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

        return new QueryLogRes(false, exc.Message);
    }
}
\$\endgroup\$
4
\$\begingroup\$

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

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

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

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

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

  5. 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<LogsDesplieguesDto> and returns a IList<DetailLog> object.

private IList<DetailLog> ToDetailLogList(IList<LogsDesplieguesDto> listLogs)
{
    IList<DetailLog> detailLogs = new List<DetailLog>();

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

Next we add a method to convert a IList<LogsDesplieguesDto> to a QueryLogRes object.

private const String noEntries = "No se han encontrado registros";
private QueryLogRes ToQueryLogResult(IList<LogsDesplieguesDto> 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<QueryLogReq>(request, "request");

        IList<LogsDesplieguesDto> 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<DefaultFaultContract>(new DefaultFaultContract(-1, auxExc.Message, Guid.Empty));
        }

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

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

        IList<LogsDesplieguesDto> 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<DefaultFaultContract>(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 classes not shown:
LogsDesplieguesDto.result seems like a property so it should be named using PascalCase like LogsDesplieguesDto.Result.The same is true for DetailLog.resultand QueryLogReq.result.

As we see in the ToDetailLog method, DetailLog and also LogsDesplieguesDto are sharing a lot of properties which could be extracted to an interface. This interface could then be used as a parameter of the DetailLog's constructor. In this way you could use the code of the ToDetailLog method in the DetailLog's constructor.

\$\endgroup\$
2
  • \$\begingroup\$ "at declaration of an boolean variable the initial value is always false" No, it's not, it doesn't have any value. (For example bool b; if (b) { } won't compile.) You might be confusing local variables with fields, which do behave the way you describe. \$\endgroup\$ – svick Sep 26 '14 at 17:28
  • \$\begingroup\$ @svick Thanks. Updated answer. I have mixed up with VB.NET. \$\endgroup\$ – Heslacher Sep 26 '14 at 17:58
3
\$\begingroup\$
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;

You can use object initializer syntax here:

var dl = new DetailLog
{
    Entorno = row.Entorno,
    Fecha = row.Fecha,
    Log = row.Log,
    Maquina = row.Maquina,
    result = row.result,
    Servidor = row.Servidor,
    Usuario = row.Usuario,
    Etiqueta = row.Etiqueta,
    PeticionEV = row.PeticionEV
};

If you do this kind of mapping often, you might want to consider using a tool like AutoMapper.

\$\endgroup\$

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.