Web service handler class

Question

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

Answer 1

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.