2
\$\begingroup\$

Please suggest how I can improve my code (like memory leakage, which pattern to use etc.). I have to parse a book XML in two ways: first from an FTP file and second from the REST API. I implemented the methods in one class.

public void GetBooksFromApi(int bookId, int bookNumber = 0)
    {
        string ApiBookIDPrefix = ConfigurationManager.AppSettings["ApiBookIDPrefix"].ToString();
        string key = ConfigurationManager.AppSettings["BookApiKey"].ToString();
        string apiBookIdValue = $"{ApiBookIDPrefix}{bookId}";
        string restApiBookUrl = $"{ApiDownloadUrl}?BookID={apiBookIdValue}&key={key}&bookNumber={bookNumber}&limit=1";
        string bookName = String.Empty;
        string clientFolderPath = String.Empty;
        string bookContent = String.Empty;

        try
        {//here to get response, parse xml and creating files process
            using (Stream xmlResponseStream = GetRestApiResponse(restApiBookUrl))
            {
                IEnumerable<XElement> xmlRecords = ParseBookXml(xmlResponseStream);
                if (xmlRecords == null)
                    throw new NullReferenceException($"No Book found to parse from rest api xml");

                foreach (var xmlRecord in xmlRecords)
                {
                    if (xmlRecord.Element("BookError") == null)
                    {
                        bookName = GetBookName(xmlRecord, bookId);
                        clientFolderPath = $@"{BaseFolder}\{bookId}";
                        bookContent = xmlRecord.ToString();

                        if (!string.IsNullOrEmpty(bookContent))
                            CreateFile(clientFolderPath, bookName, bookContent);

                    }

                }

            }

        }
        catch (Exception ex)
        {
            log.Error($"Unable to retrieve Book for {bookId.ToString()} with URL", ex);
            throw ex;
        }

    }

    public IEnumerable<XElement> ParseBookXml(Stream response)
    {
        IEnumerable<XElement> bookRecords = null;
        try
        {

            XDocument bookDoc = new XDocument();
            bookDoc = XDocument.Load(response);
            bookRecords = bookDoc.Root.Elements("bookReport").ToList();

            return bookRecords;
        }
        catch (Exception ex)
        {
            log.Error($"Unable to load  XML response from Rest Api Url .", ex);
            return bookRecords;

        }

    }

     public string GetBookName(XElement bookXmlRecord, int bookId)
    {
        string bookName = string.Empty;
        string bookNumberString = String.Empty;

        string bookNumberVal = bookXmlRecord.Element("bookNumber")?.Value;
        DateTime bookDateVal = now(); // today date
        bool validBookDate = DateTime.TryParse(bookXmlRecord.Element("BookDate")?.Value, out bookDateVal);

        if (!validBookDate)
            throw new InvalidCastException($"Unable to parse Book Date from rest api xml");


        bookNumberString = GetBookNumberString(int.Parse(bookNumberVal));

        bookName = $"Book-{bookId.ToString().PadLeft(4, '0')}-{reportDate:yyyyMMdd}{bookNumberString}.xml";

        return bookName;

    }

    public bool CreateFile(string clientFolder, string fileName, string xmlResponse)
    {
        try
        {
            string filePath = $@"{clientFolder}\{fileName}";
            if (!Directory.Exists(clientFolder))
                Directory.CreateDirectory(clientFolder);

            File.WriteAllText(filePath, xmlResponse);
            return true;
        }
        catch (Exception ex)
        {
            log.Error($"Unable to create book in path {clientFolder}.", ex);
            return false;

        }

    }
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

How to improve your code:

  1. Rather than using the XML document and XML elements, use a concrete class for books and their collection.
  2. With the help of the book class you can now to de-serialize the response of API.
  3. In addition to that, add GetBookName() and other relevant behavior to the book's class

I believe your response XML will look as follows:

<bookReport>
    <book>
        <bookId>12345</bookId>
        <bookNumber>12345</bookNumber>
        <bookDate>02/11/2021</bookDate>
    </book>
    <book>
        <bookId>12345</bookId>
        <bookNumber>12345</bookNumber>
        <bookDate>02/11/2021</bookDate>
    </book>
    <book>
        <bookId>12345</bookId>
        <bookNumber>12345</bookNumber>
        <bookDate>02/11/2021</bookDate>
    </book>
</bookReport>

Declare classes for de-serializing as follows:

public class Book
{
    public int BookId { get; set; }
    public int BookNumber { get; set; }     
    public DateTime reportDate { get; set; }
    
    // additional behavior or calculated fields here

    public string GetBookName {
        return $"Book-{BookId.PadLeft(4, '0')}-{reportDate.ToShortDateString()}{BookNumber}.xml";       
    }
}

[XmlRoot("bookReport")]  // to set a predefined xml root name
public class bookReport : List<Book>
{
    public bookReport()
    {
            
    }

}
\$\endgroup\$

This site is temporarily in read-only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .