HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Xml serialize to object

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
serializeobjectxml

Problem

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.

Solution

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.

Context

StackExchange Code Review Q#134722, answer score: 5

Revisions (0)

No revisions yet.