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

      Start asking to get answers

      Find the answer to your question by asking.

      Ask question

      Explore related questions

      See similar questions with these tags.