1
\$\begingroup\$

I'm writing a byte array at the end of file. The code works fine, but can you please suggest performance improvements?

static void Main(string[] args)
    {
        string PathToFile = "C:\\myfile.m4a";
        WriteAaray(PathToFile, GetMyList());
    }
public static List<float> GetMyList()
    {
        // This list is dynamically generated can go upto 5000 points
        var mylist = new List<float> { 4, 90, 54, 6, 5324, 423, 432, 4353, 463, 5, 24, 32, 4, 32, 43, 3, 56, 3 };
        return mylist;
    }
public static bool WriteAaray(string originalFile, List<float> myFloatPoints)
    {
        FileStream inputFileStream = null;
        inputFileStream = new FileStream(originalFile, FileMode.Open);
        var length = inputFileStream.Length;
        inputFileStream.Seek(length, SeekOrigin.Begin);
        StringBuilder mystring = new StringBuilder();
        string myImpPoints = "[MYIMPPOINTS]";
        mystring.Append(myImpPoints);
        var points = string.Join(",", myFloatPoints.Select(f => f.ToString()));
        mystring.Append(points);
        byte[] byteArray = Encoding.ASCII.GetBytes(mystring.ToString());
        inputFileStream.Write(byteArray, 0, byteArray.Length);
        inputFileStream?.Close();
        return true;
}
\$\endgroup\$
6
\$\begingroup\$

Code analysis

Let's run through your code bit by bit.

First a note about paths: use verbatim strings to disable escape sequences, so you don't need to escape those slashes: @"C:\myfile.m4a" instead of "C:\\myfile.m4a".


public static bool WriteAaray(string originalFile, List<float> myFloatPoints)
{
  • originalFile is a path, not a file (stream), and I'm not sure why it's called 'original', so I would rename it to filePath.
  • I'd also fix the typo in Aaray.

    FileStream inputFileStream = null;
    inputFileStream = new FileStream(originalFile, FileMode.Open);
  • The 'input' in inputFileStream is misleading: you're not reading from this file, you're writing to it. I'd rename it to just file.
  • There's no need for that first line: just write var file = new ... instead.
  • File streams are disposable, so it's better to use a using statement here.
  • Note that the File class contains several useful methods, such as File.Open, or the very convenient File.AppendAllText.

    var length = inputFileStream.Length;
    inputFileStream.Seek(length, SeekOrigin.Begin);
  • There's no need for that local variable (length): just pass file.Length directly into Seek.
  • Instead of manually seeking, consider opening the file with FileMode.Append. Not only does that seek to the end for you, it also creates the file if it doesn't exist yet (in your code that'll throw an exception, which will crash your program because you're not catching it).

    StringBuilder mystring = new StringBuilder();
    string myImpPoints = "[MYIMPPOINTS]";
    mystring.Append(myImpPoints);
  • While string builders are often more efficient than string concatenation, in this case you're only concatenating two strings (this header and the result of that string.Join call), so a string builder won't help here - it might even be less efficient. Note that string.Join uses a string builder internally, and with 5000 floats it certainly is useful there.
  • Again, no need for a local variable (myImpPoints) - just pass it into Append directly.

    var points = string.Join(",", myFloatPoints.Select(f => f.ToString()));
    mystring.Append(points);
  • There's no need for that Select call - one of the overloads of string.Join takes an IEnumerable<T>, so you can pass your list of floats in directly.
  • On the other hand, this only gives you 7 digits of precision. If that's a problem, then you should keep the Select part, but be sure to specify "R" (round-trip) or "G9" (recommended for performance reasons) as the to-string format.
  • Again, no need for a local variable (points).
  • Note that formatting and parsing string representations of floats is slower than writing and reading them as binary data.

    byte[] byteArray = Encoding.ASCII.GetBytes(mystring.ToString());
    inputFileStream.Write(byteArray, 0, byteArray.Length);
    inputFileStream?.Close();
  • Consider using a StreamWriter instead. That takes care of encoding and buffering, so you don't have to mess with string builder buffers and byte arrays and all that.
  • With a using statement around the file stream, you don't need to call Close manually here. Either way, you don't need an 'elvis operator' here - inputFileStream obviously won't be null.

    return true;
}
  • You're returning a boolean to indicate success, but that's actually somewhat misleading: this method can fail, but instead of returning false (which I would expect, with such a method signature) it'll throw an exception. Either catch it and return false (but then the caller won't know why it failed), or don't return anything and let the method throw (but document it).

Alternatives

5000 floats isn't a whole lot, so unless you need to create a lot of these files, the following code should be fast enough:

File.AppendAllText(filePath, "[MYIMPPOINTS]" + string.Join(",", values), Encoding.ASCII);

If, after actually measuring performance, that turns out to be too slow, then here's what your code could look like with the above notes taken into account:

using (var file = File.Open(originalFile, FileMode.Append, FileAccess.Write))
using (var writer = new StreamWriter(file, Encoding.ASCII)) // buffer size can be adjusted if necessary
{
    writer.Write("[MYIMPPOINTS]");

    if (values.Count > 0)
        writer.Write(values[0]);

    for (int i = 1; i < values.Count; i++)
    {
        writer.Write(',');
        writer.Write(values[i]);
    }
}
\$\endgroup\$
3
\$\begingroup\$

One small addition to Pieter's exhaustive answer. As all you do with myFloatPoints in your WriteAaray method is iterating, you could relax the parameter type to IEnumerable<float>.

public static bool WriteAaray(string originalFile, IEnumerable<float> myFloatPoints)

This way it can be called directly not only for List<float>, but also e.g. for float[] or the result of a LINQ expression.

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