2
\$\begingroup\$

I've implemented a class to perform a couple of tasks through a web service. I'll be really keen if you can take a look at, give me your professional opinion.

using MWM.NavisionWebService.WebReferenceTest;
using NLog;
using System;
using System.Net;
using System.Configuration;

namespace MWM.NavisionWebService
{
    public class WebServiceAccess : IDisposable
    {
        private readonly Logger _logger = LogManager.GetCurrentClassLogger();
        private readonly string _user;
        private readonly string _pass;
        private readonly NSO _nsoWebService;

    public WebServiceAccess()
    {
        _nsoWebService = new NSO { Credentials = GetCredentials() };
        _user = ConfigurationManager.AppSettings["CghWebServiceUserName"];
        _pass = ConfigurationManager.AppSettings["CghWebServicePassword"];
    }

    public void UpdateTechnician(string serviceOrderId, string technicianErpId)
    {
        try
        {
            int errorCode = _nsoWebService.UpdateResource(serviceOrderId, technicianErpId);

            if (errorCode != 0)
                HandleErrors(errorCode);
            else
                _logger.Trace(String.Format("Navision Technician updated to {0} for service order {1}.", technicianErpId, serviceOrderId));
        }
        catch (Exception ex)
        {
            _logger.Error("Error updating a technician in Navision. Error: " + ex.Message);
            throw;
        }
    }

    public void UpdateAppointmentDate(string serviceOrderId, DateTime startDate)
    {
        try
        {
            int errorCode = _nsoWebService.UpdateTime(serviceOrderId, startDate, startDate);

            if (errorCode != 0)
                HandleErrors(errorCode);
            else
                _logger.Trace(String.Format("Navision appointment updated to {0} for service order {1}.", startDate, serviceOrderId));
        }
        catch (Exception ex)
        {
            _logger.Error("Error updating an appointment in Navision. Error: " + ex.Message);
            throw;
        }
    }

    private ICredentials GetCredentials()
    {
        return new NetworkCredential(_user, _pass);
    }

    private void HandleErrors(int errorCode)
    {
        _logger.Error("Error updating Navision through the web service. Code: " + errorCode + " Message: " + GetErrorCodeDescription(errorCode));
    }

    private string GetErrorCodeDescription(int errorCode)
    {
        switch (errorCode)
        {
            case 0:
                return "OK";
            case 1:
                return "Could not find CCC Order No.";
            case 51:
                return "Could not write table.";
            default:
                return "Error code not found.";
        }
    }

    public void Dispose()
    {
        // Dispose from unmanaged resources
        _nsoWebService.Dispose();
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ Hmmm, I'm wondering if your Dispose() method does enough. \$\endgroup\$ – BCdotWEB Mar 17 '15 at 14:46
2
\$\begingroup\$

Your class will be difficult to test since you don't implement Inversion of Control. In particular due to this line in the constructor:

_nsoWebService = new NSO { Credentials = GetCredentials() };

But the same applies to Logger _logger = LogManager.GetCurrentClassLogger();: what if you want to write a unit test to check whether the correct message is logged?


There are several instances of this:

if (errorCode != 0)
   HandleErrors(errorCode);
else
   _logger.Trace(/* message */);

I'd rather see this reversed, because testing for a negative makes things more complicated than they should be IMHO (I've also added brackets, something that you should always do):

if (errorCode == 0)
{
   _logger.Trace(/* message */);
}
else
{
   HandleErrors(errorCode);
}

Considering that this log action is the only thing that happens, you could even do this:

if (errorCode == 0)
{
   _logger.Trace(/* message */);
   return;
}

HandleErrors(errorCode);

Your errorCode shouldn't be an int, but an enum with a description.


Isn't there an overload for _logger.Trace() and _logger.Error() that functions the same as string.Format()? Seems there is if you're using NLog.


NSO doesn't follow Microsoft's rules: capitalize only the first character of acronyms with three or more characters. But it is also a class name that doesn't tell me anything.

I also don't think _user and _pass should be in the WebServiceAccess class, nor GetCredentials(). These things belong to NSO.


You do not take into account that an exception may have an inner exception:

_logger.Error("Error updating a technician in Navision. Error: " + ex.Message);

HandleErrors doesn't really do any error handling, now does it? It merely logs the error.

\$\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.