No need to say what @BCdotWEB and @JanDotNet have said but in addition there something called Dependency Injection- which is avoid creating an object each time in your constructor but give it as an argument. More details Dependency Injection
public ProcedureManager()
{
this.procedureDataManager = new ProcedureDataManager();
}
should be written as
public ProcedureManager(ProcedureDataManager procedureManager)
{
this.procedureDataManager = procedureManager;
}
Note: once you use a using statement, you don't have to call a close method because using calls the Dispose method. for instance
using (XmlTextReader reader = new XmlTextReader(FileShareUrlBuilder.GetPath(Constants.Structures.PathType.XMLFILEPATH)))
{
object obj = serializer.Deserialize(reader);
this.procedure = (DtoProcedureXml)obj;
reader.Close();
}
should be written as
using (XmlTextReader reader = new XmlTextReader(FileShareUrlBuilder.GetPath(Constants.Structures.PathType.XMLFILEPATH)))
{
object obj = serializer.Deserialize(reader);
this.procedure = (DtoProcedureXml)obj;
}
alternatively usinguse try and catch so you can explicitly call the Dispose() . More Details on that https://msdn.microsoft.com/en-GB/library/yh598w02.aspx
You don't have to use this keyword in C#. You can just call the variable
procedure = (DtoProcedureXml)obj;
Also, you don't always want to catch all exceptions as you have done in SaveProcedureData() . Trying catching specific exceptions and alternatively you could log in exceptions you didn't anticipate.
catch (Exception ex)
{
CommonLogger.LogException("Failed to save procedure data");
}
Lastly, I rather refrain from names like this DtoProcedureXml as they don't give you knowledge of what the class does.