Xml serialize to object

Question

I have to read some data from xml file and then need to store the converted json in the database. The class DtoProcedureXml mapped with xml attributes.

public class ProcedureManager
{
    #region Declaration
    private DtoProcedureXml procedure;
    private ProcedureDataManager procedureDataManager;
    #endregion

    public ProcedureManager() 
    {
        this.procedureDataManager = new ProcedureDataManager();
    }

    public bool SaveProcedureData(long procedureID)
    {
        bool isSuccess = false;
        try
        {
            SetProcedureXmlData();
            isSuccess = SaveProcedureSummary(procedureID);
        }
        catch (Exception ex)
        {
            CommonLogger.LogException("Failed to save procedure data");
        }
        return isSuccess;
    }


    private bool SaveProcedureSummary(long procedureID)
    {
        //saving the data to db
        return procedureDataManager.Save(new DtoProcedure { ProcedureID = procedureID, JsonData = Newtonsoft.Json.JsonConvert.SerializeObject(this.procedure), CreatedOn = DateTime.Now });
    }

    private void SetProcedureXmlData()
    {
        this.procedure = new DtoProcedureXml();
        XmlSerializer serializer = new XmlSerializer(typeof(DtoProcedureXml));
        using (XmlTextReader reader = new XmlTextReader(FileShareUrlBuilder.GetPath(Constants.Structures.PathType.XMLFILEPATH)))
        {
            object obj = serializer.Deserialize(reader);
            this.procedure = (DtoProcedureXml)obj;
            reader.Close();
        }
    }
}

I have some concerns with the function SetSReportXmlData() When I was debugging the XmlSerializer code takes few milliseconds, is it right to move that code in the using section? Also please let me know if there is any efficient solutions than this logic.

Community · Accepted Answer · 2017-04-12 07:31:52Z

Some quick remarks:

You don't need regions in a 50 line class. In fact, avoid regions.
Why do you store procedureDataManager and procedure on the class? Why not instead have SetProcedureXmlData() return a DtoProcedureXml?
No need to shout: XMLFILEPATH. Constants should be PascalCased.
SaveProcedureData can fail, so perhaps it should be named something like TrySaveProcedureData.
You swallow the Exception and never even log it anywhere? Don't you want to know what has gone wrong and try to avoid the issue in the future?
Avoid unnecessary comments like //saving the data to db. Your code should tell me that.
I don't see the point of SaveProcedureSummary as a separate method: it contains one line and is called only once.

JanDotNet · Accepted Answer · 2016-07-13 10:20:20Z

I would suggest not to recreate the instance of XmlSerializer each time, because it creates an assembly in memory that is not reused / unloaded. To avoid that, just create the instance as static field of the class or use something like a XmlSerializerCache.

see also: XmlSerializer class may result in a memory leak and poor performance

The value of procedure could be returned by SetProcedureXmlData and passed to SaveProcedureSummary. That makes it more clear the the methods have to be called in that order.
The instance variable procedureDataManager could be readonly
You could drop the isSuccess variable if you return the result of SaveProcedureSummary or false in the catch block.
It would be more readable if the line in SaveProcedureSummary is splitted to 2 or 3 lines.
The using is OK, but ther is no need to close the reader because it will be closed when the reader is disposed by the using.

Tolani · Accepted Answer · 2016-07-13 13:35:08Z

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.

Stack Exchange Network

Xml serialize to object

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Xml serialize to object

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions