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\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.