8
\$\begingroup\$

I'm pretty sure this code can be optimized, but I'm not talented enough in Linq to do it myself. Here's what I'm trying to do: I have an XML file that needs to be converted into a .csv file. The XML looks like this:

<Inventory>
    <Item>
        <Name>Super Mario Bros</Name>
        <Count>14</Count>
        <Price>29,99</Price>
        <Comment>-No Comment-</Comment>
        <Artist>N/A</Artist>
        <Publisher>Nintendo</Publisher>
        <Genre>Video Games</Genre>
        <Year>1985</Year>
        <ProductID>001</ProductID>
    </Item>
    <Item>
        <Name>The Legend of Zelda</Name>
        <Count>12</Count>
        <Price>34,99</Price>
        <Comment>-No Comment-</Comment>
        <Artist>N/A</Artist>
        <Publisher>Nintendo</Publisher>
        <Genre>Video Games</Genre>
        <Year>1986</Year>
        <ProductID>002</ProductID>
    </Item>
</Inventory>

(There are many more Items in the list, but they are all the same.)

The code I'm currently using is working as intended, here it is:

public void fileConvert_XMLToCSV() {
    //This method converts an xml file into a .csv file

    XDocument xDocument = XDocument.Load(FilePath_CSVToXML);
    StringBuilder dataToBeWritten = new StringBuilder();

    var results = xDocument.Descendants("Item").Select(x => new {
        title = (string)x.Element("Name"),
        amount = (string)x.Element("Count"),
        price = (string)x.Element("Price"),
        year = (string)x.Element("Year"),
        productID = (string)x.Element("ProductID")
    }).ToList();

    for (int i = 0; i < results.Count; i++) {
        string tempTitle = results[i].title;
        string tempAmount = results[i].amount;
        string tempPrice = results[i].price;
        string tempYear = results[i].year;
        string tempID = results[i].productID;

        dataToBeWritten.Append(tempYear);
        dataToBeWritten.Append(";");
        dataToBeWritten.Append(tempTitle);
        dataToBeWritten.Append(";");
        dataToBeWritten.Append(tempID);
        dataToBeWritten.Append(";");
        dataToBeWritten.Append(tempAmount);
        dataToBeWritten.Append(";");
        dataToBeWritten.Append(tempPrice);
        dataToBeWritten.Append(";");
        dataToBeWritten.Append(0);
        dataToBeWritten.Append(";");
        dataToBeWritten.Append(0);
        dataToBeWritten.Append(Environment.NewLine);
    }

    Console.WriteLine(dataToBeWritten.ToString());
    Console.ReadLine();

    var testpath = AppDomain.CurrentDomain.BaseDirectory + @"frMediaShop\test.csv";

    File.WriteAllText(testpath, dataToBeWritten.ToString());
}

Running this method outputs a file (test.csv) that looks just like I want it. But the code is repetitive and dull. Please help me optimize it.

\$\endgroup\$
1
  • \$\begingroup\$ Instead of working the format translation manually, I recommend you to look into existing libraries. You will be surprised how many details exist even for such a simple format as CSV... Why taking all this burden? Somebody has done all the hard work for you. I'd look into this library: github.com/ServiceStack/ServiceStack.Text \$\endgroup\$ Commented Apr 7, 2017 at 17:52

4 Answers 4

9
\$\begingroup\$

First of all, I'd split the convert method out into it's own thing - separate from the loading and saving:

// Load xml
XDocument xDocument = XDocument.Load(FilePath_CSVToXML);

// Convert
string data = Convert(xDocument);

// Do whatever it is you want to do with the results
Console.WriteLine(data);
Console.ReadLine();

var testpath = AppDomain.CurrentDomain.BaseDirectory + @"frMediaShop\test.csv";

File.WriteAllText(testpath, data);

We can simplify the actual conversion by using string interpolation and rolling it all up in a single LINQ statement:

    private static string Convert(XDocument xDocument)
    {
        var data = new StringBuilder();

        foreach (var result in xDocument.Descendants("Item").Select(x => new {
            title = (string)x.Element("Name"),
            amount = (string)x.Element("Count"),
            price = (string)x.Element("Price"),
            year = (string)x.Element("Year"),
            productID = (string)x.Element("ProductID")
        }))
        {
            data.AppendLine($"{result.year};{result.title};{result.productID};{result.amount};{result.price};{0};{0}");
        };

        return data.ToString();
    }
\$\endgroup\$
4
  • \$\begingroup\$ This is an interasting casing LinQ ;-) \$\endgroup\$
    – t3chb0t
    Commented Apr 5, 2017 at 15:15
  • \$\begingroup\$ Yeah, dunno where that came from! \$\endgroup\$
    – Pj-
    Commented Apr 5, 2017 at 15:21
  • 2
    \$\begingroup\$ @Pj. - Wouldn't it be better to create the string in the select statement, rather than creating anonymous objects first? \$\endgroup\$
    – user33306
    Commented Apr 5, 2017 at 17:32
  • \$\begingroup\$ Although seperating the steps like this does increase readability, it causes the full document to be loaded into memory multiple times (As an XDocument, as a StringBuilder and as a string) which would cause significant issues if the file is large. \$\endgroup\$
    – Wazner
    Commented Apr 5, 2017 at 19:23
4
\$\begingroup\$

Some of the values contain spaces so to make the csv file more robust and easier to read you should quote them or just everything. I also noticed that your csv does not contain the header row. Is there a reason for that?

I suggest using a more generic approach with some more linq that will first extract header names and then the values. Everything is joined together with the Aggregate extension that uses a StringBuilder initialized with the header and then appends each line to it.

var headers =
    xDocument
        .Descendants("Item")
        .First()
        .Elements().Select(e => e.Name.LocalName);

var delimiter = ";";

var entries =
    xDocument
        .Descendants("Item")
        .Select(d => string.Join(delimiter, d.Elements().Select(e => $"\"{e.Value}\"")))
        .Aggregate(
            new StringBuilder().AppendLine(string.Join(delimiter, headers)), 
            (current, next) => current.AppendLine(next));

var csv = entries.ToString();

There is no need to hardcode the field names.

\$\endgroup\$
4
  • \$\begingroup\$ looks great, but hard to understand.... \$\endgroup\$ Commented Feb 23, 2018 at 10:11
  • \$\begingroup\$ @t3chbot It works great when you have nice and tidy XML elements repeat in sequence. +1 for the way you use LINQ \$\endgroup\$ Commented Mar 18, 2018 at 23:29
  • \$\begingroup\$ Problem with this is what if you don't have data for every element. It will throw off the csv. \$\endgroup\$
    – user890332
    Commented Jan 25, 2019 at 16:20
  • \$\begingroup\$ @user890332 probably yes... as always, reviews are just examples and usually not production-ready code ;-] \$\endgroup\$
    – t3chb0t
    Commented Jan 25, 2019 at 16:21
2
\$\begingroup\$

Your code has several issues, which may or may not apply to your case.

  • You don't sanitize any of the input, so if for example one of the XML tags contains a semicolon or a new line character you will end up with invalid CSV.
  • You load the whole document into memory (the XDocument) and then aggregate all the output into memory before writing it to a file.

I suggest a streaming approach using XmlReader and C# iterators. The example I'm giving below is more efficient, but may be too complex for the task you require (small files).

First the reader and converter

class InventoryItem 
{
    // Properties for title, amount, price, year, etc..
}

private IEnumerable<InventoryItem> LoadInventoryItemsFromXML(string xmlFile) 
{
    // Open the source XML file
    using (var stream = new FileStream(xmlFile, FileMode.Open, FileAccess.Read, FileShare.Read))
    using (var reader = XmlReader.Create(stream)) 
    {
        // Read each of the tags
        // MoveToContent skips any whitespace and comments that may reside in the XML

        reader.MoveToContent();
        reader.ReadStartElement(); // Inventory
        reader.MoveToContent();

        while (reader.NodeType == XmlNodeType.Element)
        {
            reader.ReadStartElement(); // Item

            // Read the individual properties of the inventory item
            var item = new InventoryItem();
            while (reader.NodeType == XmlNodeType.Element)
            {
                // You could use a switch, possibly a dictionary if the amount of fields increase
                // Or you could use reflection
                switch (reader.LocalName)
                {
                    case "Name":
                        item.Name = reader.ReadElementContentAsString();
                        break;

                    case "Count":
                        item.Count = reader.ReadElementContentAsInt();
                        break;

                    // etc.  

                    // Skip any unkown properties
                    default:
                      reader.Skip();
                      break;
                }
            }

            reader.ReadEndElement(); // Item

            yield return item;
        }

        reader.ReadEndElement(); // Inventory
    }
}

Then write result out to CSV

private void WriteInventoryItemsToCSV(string csvFile, IEnumerable<InventoryItem> items)
{
    // Open the output CSV file
    using (var stream = new FileStream(csvFile, FileMode.Create, FileAccess.Write, FileShare.None))
    using (var writer = new StreamWriter(stream)) 
    {
        // Loop through each of the items we read from the source CSV file.
        foreach (var item in items) 
        {
            writer.Write(CSVEscape(item.Title));
            writer.Write(';');

            // Asumming Amount and Price are numbers, we know they won't contain semicolons.
            // This means we don't have to escape them.
            writer.Write(item.Amount);
            writer.Write(';');
            writer.Write(item.Price);          
            writer.WriteLine();
        }
    }
}

// This methods escapes a string using quotes
private string CSVEscape(string str) 
{
    // Surround content in quotes and replace any quotes inside the string
    // with two double quotes
    return "\"" + str.Replace("\"", "\"\"") + "\"";
}

Then you can call this where you need it

var items = LoadInventoryItemsFromXML(FilePath_CSVToXML);
WriteInventoryItemsToCSV(AppDomain.CurrentDomain.BaseDirectory + @"frMediaShop\test.csv", items);
\$\endgroup\$
2
\$\begingroup\$

You can do it with a series of Regex.Replaces:

// 1) Replace closing and opening tags with commas.
//    Include quotes in case any values have commas in them.
var result = Regex.Replace(input, @"(\S)<\/[^>]*>\s*<[^>]*>(\S)", "$1\",\"$2");

// 2) Put in CSV line breaks and remove xml delimiters, include leading and trailing quotes
result = Regex.Replace(result, @"<\/[^>]*>\s*<\/Item>\s*<Item>\s*<[^>]*>", "\r\n");

// 3) Remove remaining tags and trim any whitespace
result = Regex.Replace(result, @"\s*<.*>\s*", "");

// 4) put in header row and first and last quotes
result = "Name,Count,Price,Comment,Artist,Publisher,Genre,Year,ProductID,\r\n\"" + 
         result + "\"";

is this a better solution than other solutions? No. It's brittle and would break easily.

Here are the Regex steps visually:

  1. https://regex101.com/r/2jmvhc/1
  2. https://regex101.com/r/0XyvZe/1
  3. https://regex101.com/r/Sj6GsP/1
\$\endgroup\$
1
  • \$\begingroup\$ Gave this plus one even though this is clearly not advisable but for the great regex example. \$\endgroup\$
    – user890332
    Commented Jan 24, 2019 at 20:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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