2
\$\begingroup\$

I am interested in learning a more succinct (or better performing) way of writing the following working code. I just figured it out but it is pretty messy. This program takes a file full of daily financial transactions for x number of months (for example), and appends each line to the appropriate month file to create separate monthly reports. I put the open month files in a list. If the filestream is not yet created, I create it, add it to the list, then append the data. If it is already created/open, I append the data to it.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;

namespace SplitFiles
{
    class Program
    {
        private static FileStream fs;
        private static List<FileStream> lf;
        private static StringBuilder sb;
        private static string[] header;
        private static string _file;

        private static string _fileName;

        private static string _startDir;

        private static string _newFile;
        public static string NFile
        {
            get { return _newFile; }
            set
            {
                _newFile = Path.Combine(_startDir, $"{value}_{_fileName}");
            }
        }

        static byte[] ReturnBytes(string s)
        {
            return Encoding.ASCII.GetBytes(s);
        }

        static void CeateFile(string monyr)
        {
            NFile = monyr;
            fs = new FileStream(NFile, FileMode.Create);
            for(var i = 0; i < 3; i++)
            {
                var b = ReturnBytes($"{header[i]}{Environment.NewLine}");
                fs.Write(b, 0, b.Length);
            }

            lf.Add(fs);
        }

        static void Main(string[] args)
        {
            sb = new StringBuilder();
            header = new string[4];
            _startDir = @"D:\ProgramData\MonthlyReports";
            _file = @"D:\ProgramData\test.txt";
            _fileName = Path.GetFileName(_file);
            lf = new List<FileStream>();

            var my = new string[2];

            DateTime dt;

            // if directory doesnt exist, create it
            if (!Directory.Exists(_startDir)) Directory.CreateDirectory(_startDir);

            using (var sr = new StreamReader(File.OpenRead(_file)))
            {
                var i = 0;

                while (!sr.EndOfStream)
                {
                    sb.Append(sr.ReadLine());

                    // first 3 lines of file contain:
                    // -------- etc
                    // field1 | field2 | field3 | DATEFIELD | field5 | etc. | etc.
                    // -------- etc

                    // last line: ------------------------ etc

                    // Build header array (first 3 lines = header, last line = footer)
                    if (i < 3 || sb.ToString().Contains("-------------"))
                    {
                        header[i] = sb.ToString();
                        i++;
                        sb.Clear();
                        continue;
                    }

                    // split line by bar | delimiter
                    var pop = sb.ToString().Split('|');
                    if(DateTime.TryParse(pop[4], out dt))
                    {
                        my[0] = dt.Month.ToString();
                        my[1] = dt.Year.ToString();

                        fs = lf.FirstOrDefault(a => a.Name.IndexOf($"{my[0]}_{my[1]}") != -1);

                        // if fs is null, create filestream and set fs = new filestream
                        if (fs == null) CeateFile($"{my[0]}_{my[1]}");
                        var b = ReturnBytes($"{sb.ToString()}{Environment.NewLine}");
                        fs.Write(b, 0, b.Length);
                    }

                    sb.Clear();
                }
            }

            var finalLine = ReturnBytes($"{header[3]}{Environment.NewLine}");
            lf.ForEach(a =>{
                a.Write(finalLine, 0, finalLine.Length);
                a.Close();
            });
            sb.Clear();
            sb = null;
            fs.Close();
        }
    }
}
| improve this question | | | | |
\$\endgroup\$
4
\$\begingroup\$

Problems I see

  • Having all methods static will envolve some problems you can avoid:

    • the code is harder to test
    • using static methods with a class having a state will lead to problems if multi threading should be introduced
  • Creating filestreams which are kept open can lead to problems because in the case of an exception the streams aren't properly disposed.

  • Inconsistent naming of variables. Either use underscore-prefixed names or don't use them. Mixing styles will lead to harder to read and therefor harder to maintain code. Prefixing static variables with an underscore is IMO not correct.

  • The names of most variables don't tell the reader of the code anything about their purpose. Always make the names of things as descriptive as possible. E.g private static List<FileStream> lf; would be named better fileStreams.. If you or Sam the maintainer come back in 2 months to this code because you either have a bug or you need to add a feature you would have a hard time to figure out what all the variables and parameters (like string monyr) represent hence your task will take a lot more time.

  • If the file isn't that big (not megabytes) simply using File.ReadAllLines() would be better, because it would remove the need to convert the bytes to text because that is done by that said method and it will shorten your code.

  • If the file is big, you should consider to use a TextReader instead of a StreamReader because the TextReader is doing the converting stuff under the hood.

  • If you are using comments you should use them only if it isn't obvious why the code does something in a specific way. Something like

            // if directory doesnt exist, create it
            if (!Directory.Exists(_startDir)) Directory.CreateDirectory(_startDir);  
    

    is stating the obvious and in addition could be just replaced by a simple Directory.CreateDirectory(_startDir); because under the hood the CreateDirectory() is checking if the directory exists before it tries to create it.

| improve this answer | | | | |
\$\endgroup\$
2
\$\begingroup\$

Might I add, using a collection of file names instead of a collection of FileStreams would be more efficient, since your code only seems to deal with one file at a time. You could use the built in collection in the IO.Directory class.

Also checking if a file exists before you create it, is redundant since using FileMode.Append will open or create and open the file. Also getting the collection of filenames directly from the directory will keep it up to date.

| improve this answer | | | | |
\$\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.