0
\$\begingroup\$

Following a question I asked and the advice on SO I implemented the following solution. Are there any downsides to such a solution? Is there a better way to make it changeable when I need to control formatting of other types like double and float where I have more than one occurrence of each in the dataLine[]?

    public void Save(DataTable newMyDataTable)
    {
        String dateTimeFormat = "yyyy-MM-dd HH:mm:ss";
        try
        {
            lock (writtingLock)
            {
                Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;
                dataTable = newMyDataTable;
                fileId = dataTable.TableName;
                PathMaker path = new PathMaker(fileId);
                path.Make();
                // writing the table to a file
                using (StreamWriter swr =
                    new StreamWriter(File.Open(path.filePath, FileMode.Create), Encoding.Default, 1000000))
                // change buffer size and Encoding to your needs
                {
                    if (addHeader)
                    {
                        foreach (var dc in dataTable.Columns)
                        {
                            swr.Write(dc.ToString() + ",");
                        }
                        swr.WriteLine();
                    }
                    foreach (DataRow dr in dataTable.Rows)
                    {
                        Object[] dataLine = dr.ItemArray;
                        // extract date time and format it
                        var dateTime = dataLine.Where(o => o.GetType() == typeof(DateTime)).Select(t => (DateTime)t).Select(x => x.ToString(dateTimeFormat)).ToArray();
                        // collect data other than date time
                        var otherData = dataLine.Where(o => o.GetType() != typeof(DateTime)).ToArray();
                        // build your string
                        string dateTimeToWrite = string.Join(",", dateTime);
                        string otherDataToWrite = string.Join(",", otherData.Select(x => x.ToString().TrimEnd(null)).ToArray());
                        string lineToWrite = dateTimeToWrite + "," + otherDataToWrite;
                        swr.WriteLine(lineToWrite);
                    }
                }
            }
        }
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

What's the point of dataTable = newMyDataTable;? Why not simply work with newMyDataTable directly? Note that newMyDataTable is a bad variable name anyway.


Ditto fileId: this is only used to pass to PathMaker, so why then not simply use newMyDataTable.TableName?


Ditto dataLine: why not work with dr.ItemArray directly?


writtingLock has a spelling error.


What is dc? Ditto dr? Use descriptive names; using abbreviations isn't making your code faster.


Why assign dateTime and otherData when you could easily make this one line? Even better: why not move this code -- including the string.Join -- to separate methods?

Example:

string.Join(",", dr.ItemArray
     .Where(o => o.GetType() == typeof(DateTime))
     .Select(x => ((DateTime)x).ToString(dateTimeFormat)));

Why assign lineToWrite when you could simply write swr.WriteLine(dateTimeToWrite + "," + otherDataToWrite);?


filePath is a property, so it should be PascalCase.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks, great suggestions, i will follow them, But i am not sure how to one line the dateTime and otherData ? can you show me how ? i am very new to LINQ and C# (4 months) \$\endgroup\$
    – InsaneBot
    Dec 9, 2015 at 15:09
  • \$\begingroup\$ Another question about the property having a PascalCase, where can i find all the naming conventions ? googling reveal sooo many opinions, what do you use ? \$\endgroup\$
    – InsaneBot
    Dec 9, 2015 at 15:43
  • \$\begingroup\$ @BinaryA msdn.microsoft.com/en-us/library/ms229002.aspx , msdn.microsoft.com/en-us/library/xzf533w0.aspx \$\endgroup\$
    – BCdotWEB
    Dec 9, 2015 at 15:54
1
\$\begingroup\$

Your time format should be a constant. After all, it isn't dependant on any other code in your method.

private const string DateTimeFormat = "yyyy-MM-dd HH:mm:ss";

There's wayyy too much things in that lock. Locks need to be used cautiously, you don't want to bottleneck your application with a lock. Try to remove everything that's not related to the thread-safe operation from the lock.

And well, looking at your code, nothing needs thread safety here. Maybe your "write" operation needs to be thread-safe so you don't write in the same file with two different threads, but that's all. And even there, in such a case I don't think a lock is the good solution.

You should use the is operator instead of comparing types like you're doing actually :

o.GetType() == typeof(DateTime)

becomes

o is DateTime

Better eh?

Finally, most of the comments are kind of useless. Stuff like // build your string can be deduced by looking at your code! :)

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