Skip to main content
deleted 2 characters in body
Source Link
Tolani
  • 2.5k
  • 7
  • 31
  • 49

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.

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

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

Source Link
Tolani
  • 2.5k
  • 7
  • 31
  • 49

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