5
\$\begingroup\$

I'm having to process an Amazon orders report (TSV flat file from the MWS Report API) and am needing to split the report based on sales channels (I could request them separately but need to keep API request counts as low as possible) whilst editing the contents of one of the columns (mapping a virtual SKU to its real counterpart). I'm using the CsvHelper library to handle the reading and writing, and would like to handle the file a line at a time to avoid loading the entire file into memory at once, which means as far as I know I can't use the using block on the StreamWriter and CsvWriter instances, so I made a wrapper class that implements IDisposable and calls Dispose on both writers as part of its Dispose method. I store a Dictionary of these classes using sales channel as the key, then once the file has been completed I loop through the dictionary calling Dispose on each element.

Wrapper Class:

internal class AmazonCsvWriter : IDisposable
{
    private bool _disposed;
    private readonly StreamWriter _streamWriter;
    private readonly CsvWriter _csv;

    public AmazonCsvWriter(string folder)
    {
        if (!Directory.Exists(folder))
        {
            Directory.CreateDirectory(folder);
        }

        _streamWriter = new StreamWriter(Path.Combine(folder, $"{DateTime.Now:yyyyMMddHHmmss}.txt"), false, Encoding.GetEncoding("Windows-1252"));
        _csv = new CsvWriter(_streamWriter);
        _csv.Configuration.Delimiter = "\t";
        _csv.Configuration.RegisterClassMap(new AmazonOrderRowMap());
        _csv.WriteHeader<AmazonOrderRow>();
        _csv.NextRecord();
    }

    public void WriteRecord(AmazonOrderRow record)
    {
        _csv.WriteRecord(record);
        _csv.NextRecord();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual  void Dispose(bool disposing)
    {
        if (_disposed)
        {
            return;
        }

        if (disposing)
        {
            _csv.Dispose();
            _streamWriter.Dispose();
        }

        _disposed = true;
    }
}

Used in this function:

    public void ProcessFbaOrders(string reportFile)
    {
        try
        {
            using (var sr = new StreamReader(reportFile, Encoding.GetEncoding("Windows-1252")))
            using (var csv = new CsvReader(sr))
            {
                csv.Configuration.Delimiter = "\t";
                csv.Configuration.RegisterClassMap(new AmazonOrderRowMap());
                IEnumerable<AmazonOrderRow> records = csv.GetRecords<AmazonOrderRow>();
                foreach (AmazonOrderRow record in records)
                {
                    if (!_csvWriters.ContainsKey(record.SalesChannel))
                    {
                        _csvWriters.Add(record.SalesChannel,
                            new AmazonCsvWriter(Path.Combine(_platform.OutputFolder, record.SalesChannel)));
                    }

                    record.Sku = GetRealSku(record.Sku);

                    _csvWriters[record.SalesChannel].WriteRecord(record);
                }

            }
        }
        finally
        {
            foreach (KeyValuePair<string, AmazonCsvWriter> kvp in _csvWriters)
            {
                kvp.Value.Dispose();
            }
        }

        File.Delete(reportFile);
    }

_csvWriters is defined as:

    private readonly Dictionary<string, AmazonCsvWriter> _csvWriters;

Is this safely closing the Stream/CsvWriters or is there a better method for handling this?

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

First I would like to say that your code looks clean and well structured. You are doing most things well like using the proper casing for naming your things, using using's and stacking them as well, using readonly where you should.

Improvements could be made by using better names for your variables like e.g _csv and csv. A reader of your code who is switching between the AmazonCsvWriter class and the ProcessFbaOrders() method will get confused because in the AmazonCsvWriter class the _csv variable holds an instance of a CsvWriter and in the mentioned method the csv variable holds an instance of a CsvReader.

You are using a Dictionary<string, AmazonCsvWriter> to write the the unordered records to their desired destination based on the SalesChannel of the record.

Although your code accomplish its goal you could do better by using Enumerable.GroupBy() like so

IEnumerable<IGrouping<string, AmazonOrderRow>> query =
        records.GroupBy(record => record.SalesChannel, record=> record);  

and then you can iterate over it like so

foreach (IGrouping<string, AmazonOrderRow> recordGroup in query)
{
    string csvOutputPath = Path.Combine(_platform.OutputFolder, recordGroup.Key);
    Directory.CreateDirectory(csvOutputPath);

    using (var streamWriter = new StreamWriter(Path.Combine(csvOutputPath, $"{DateTime.Now:yyyyMMddHHmmss}.txt"), false, Encoding.GetEncoding("Windows-1252")))
    using (var csvWriter= new CsvWriter(streamWriter))
    {
        csvWriter.Configuration.Delimiter = "\t";
        csvWriter.Configuration.RegisterClassMap(new AmazonOrderRowMap());
        csvWriter.WriteHeader<AmazonOrderRow>();
        csvWriter.NextRecord();
        foreach(var record in recordGroup)
        {
            record.Sku = GetRealSku(record.Sku);
            csvWriter.WriteRecord(record);
            csvWriter.NextRecord();
        }
    }
}

As a side note, you don't need to check if a directory exists before you call Directory.CreateDirectory() because the CreateDirectory() method checks the same and if the directory exists it simply returns.

\$\endgroup\$
5
  • \$\begingroup\$ I suspect using GroupBy results in reading the whole collection into memory. The OP is making one pass when reading and then writing and is basically holding on to only one line at a time which would be more memory efficient. Grouping would require loading everything into memory which, if the file is large, could result in unstable behavior. \$\endgroup\$
    – Nkosi
    Dec 28, 2017 at 7:21
  • \$\begingroup\$ @Nkosi you are suspecting correctly about GroupBy. Assume the file has 50000 rows with 5000 different SalesChannel this would create a Dictionary containing 5000 CsvWriter and StreamWriter objects which would consume a lot of memory as well. Without knowing the size of the file and the count of different SalesChannel I will stay with my answer. \$\endgroup\$
    – Heslacher
    Dec 28, 2017 at 7:46
  • 1
    \$\begingroup\$ @Heslacher Thanks for your answer. In regards to the sales channels (basically which Amazon TLD the order was placed through) I'm currently only looking at a max of 5 and can't see it going much higher than 10. \$\endgroup\$ Dec 28, 2017 at 9:26
  • \$\begingroup\$ How many rows are in the file ? \$\endgroup\$
    – Heslacher
    Dec 28, 2017 at 9:27
  • 1
    \$\begingroup\$ @Heslacher My current test file is around 2000 lines, with 48 columns of data. I'm probably prematurely optimising, but with the test file the GroupBy method is using ~4MB more memory than my existing method. \$\endgroup\$ Dec 28, 2017 at 9:33
3
\$\begingroup\$

A few notes:

  • Your class does not contain a finalizer, so there's no use in calling GC.SuppressFinalize(this).
  • The disposing foreach loop can be simplified to foreach (var writer in _csvWriters.Values) { writer.Dispose(); }.
  • You're not clearing the _csvWriters dictionary after disposing its contents.
\$\endgroup\$

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.