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

• BCdotWEB: Hmmm, I'm wondering if your Dispose() method does enough.

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.