Replace strings in a file

Question

I had to create an executable to search and replace strings in a file. This is to be used in my installer for text file manipulation. I have various placeholders in configuration files that I need to replace with proper values after querying the end user system.

I have written the following code. This being my first real experience with C#, I do not expect the code to be any good. I have come up with this code based upon a couple of hours of reading MSDN and stackoverflow. I needed someone to review this. There is no one greater than the community to do this.

Please review this and let me know what modification are necessary. I am specifically concerned about the exception handling. I have included two try-catch blocks.

My logic:

If I have all operations under one try-catch block, the failure of one process will block the execution of all others. That is, if my

text = Regex.Replace(text, args[1], args[2]);

has an exception and if I have

WriteLog(args[0], args[1], args[2], strException, intStatus);

also in the same try block, it will not execute and I will not have any logs.

Also, if I have the

WriteLog(args[0], args[1], args[2], strException, intStatus);

in the catch block, any exception in that method cannot be caught. Yes I can have try-catch in the WriteLog method but I see no problems in my approach too.

So first, I have a try-catch for file manipulation and then another for log write.

Please ignore the log path(hardcoded as D:).

class Program
{
    static int Main(string[] args)
    {
        //args[0] = The file that needs modification
        //args[1] = The string to replace
        //args[2] = The string with which to replace args[1]
        int intStatus = 0;
        string strException = "";

        //Get out immediately if no/improper arguments are found
        if (args.Length < 3)
        {
            intStatus = 1;
            return intStatus;
        }

        try
        {
            StreamReader streamReader = new StreamReader(args[0]);
            String text = streamReader.ReadToEnd();
            streamReader.Close();
            text = Regex.Replace(text, args[1], args[2]);
            StreamWriter streamWriter = new StreamWriter(args[0]);
            streamWriter.Write(text);
            streamWriter.Close();
        }
        catch (Exception ex)
        {
            //text file manipulation failed
            strException = ex.ToString();
            intStatus = 2;
        }

        try
        {
            WriteLog(args[0], args[1], args[2], strException, intStatus);
        }
        catch
        {
            //log write failed
            if (intStatus == 0)
            {
                intStatus = 3; //if intStatus = 1 when we get here, no need to modify the value(its a complete failure)
                               //if intStatus = 0 when we get here, make intStatus = 2 so as to clearly distinguish a "log write failed"
                               //error from a "text file manipulation failed" error
            }
        }

        return intStatus;
    }

    //The new Logger for the exe
    static void WriteLog(string arg0, string arg1, string arg2, string exception, int status)
    {
            string strGrepLogFileName = string.Format("D:\\TempLogs\\Grep-{0:yyyy-MM-dd_hh-mm-ss-tt}.log", DateTime.Now);
            StreamWriter GrepLog = new StreamWriter(strGrepLogFileName, true);
            GrepLog.WriteLine("Argument 1: " + arg0);
            GrepLog.WriteLine("Argument 2: " + arg1);
            GrepLog.WriteLine("Argument 3: " + arg2);
            GrepLog.WriteLine("Status: " + status.ToString());
            GrepLog.WriteLine("Exception: " + exception);
            GrepLog.Close();
    }
}

EDIT 1: It is an absolute pity that I cannot mark multiple answers.

Answer 1

Use the params keyword

//The new Logger for the exe
static void WriteLog(string exception, int status, params string[] arguments)
{
    string logFile = string.Format("D:\\TempLogs\\Grep-{0:yyyy-MM-dd_hh-mm-ss-tt}.log", DateTime.Now);
    StreamWriter grepLog = new StreamWriter(logFile, true);
    grepLog.WriteLine("Status: " + status.ToString() + " (0 for PASS, 1 for FAIL)");
    grepLog.WriteLine("Exception: " + exception);
    // TODO: Consider logging "no arguments" if that is the case.
    for(int i=0; i<arguments.Length; i++)
    {
        grepLog.WriteLine("Argument " + i + ": " + arguments[i]);
    }
    grepLog.Close();
}

Use an enum for status

Since the status has 3 states, use an enum for it. This will help you avoid mistakes.

Why do you try-catch while logging?

If you don't see any particular possibility for the log to fail, then simply assume it and don't try-catch the logging.

If you want the logging to no matter what not break the application with exceptions, and you cannot fix those exceptions (why?), then I would suggest putting the try-catch inside the logging method.

Main - exception should catch, log, and return

I would have the exception log the error and return, instead of continuing normally.

Alternatively, I would catch, log, and re-throw: try { ... } catch (Exception e) { WriteLog(...); throw; }

Use camelCase for local variables

StreamWriter grepLog;
Using a starting underscore (_grepLog) is also common for private fields.

Follow the de-facto conventions. (See e.g. https://stackoverflow.com/questions/2976627/on-c-sharp-naming-conventions-for-member-variables )

Avoid Hungarian Notation

In string strGrepLogFileName, the part "str" brings nothing new.
If you change the variable's type (e.g. to an Uri) without changing its meaning, you should not have to rename the variable, and using Hungarian Notation you do.

Answer 2

To add some style pointers:

• string should be used consistently over String. Some also prefer the use of string.Empty over an empty literal(""). See StyleCop

• If the logging function isn't expected to fail, then handle everything internally to that method. Additionally, take advantage of a using block to handle the resource:

  using(StreamWriter grepLog = new StreamWriter(fileName, true))
  {
      // ...log code here
  }
  

• Your resources in the first try block will not close if they fail to read/write. Either combine a using in there with the try or close them in a finally block. I would suggest adding an additional pre-condition that verifies that the file exists before attempting to read it:

  if(!File.Exists(args[0]))
  {
      // Log error and return
  }
  

Answer 3

None of the other answers touch upon this...

Your main function does too much. Specifically, the main function should ONLY do your initial validation -- do you have the right number of parameters? If possible verify that those parameters are valid. Then call a method that does the actual work.

Code reuse, whether that is done via copy/paste, inheritance or a library, works best when you have discrete methods, that have little to no dependencies.

In addition, given your usage I would change your writelog function to TryWriteLog and have it returns success or failure (ie it eats any exceptions and then returns an appropriate boolean). Exceptions should be handled at the earliest point that knows what to do in order to recover or continue. Given that you aren't doing anything with it, handling it in the logging function makes the most sense.

Answer 4

The biggest tip I can give is to use the using construct to make sure your IDisposable resources are properly disposed. These are your StreamReaders and StreamWriters. I also think what ANeves says is spot-on for the stylistic pieces. Here's a cut:

internal static class Program
{
    private enum Status
    {
        Success,

        ImproperOrNoArgumentsFound,

        TextFileManipulationFailed,

        LogWriteFailed
    }

    private static int Main(string[] args)
    {
        // args[0] = The file that needs modification
        // args[1] = The string to replace
        // args[2] = The string with which to replace args[1]
        var status = Status.Success;
        var exception = string.Empty;

        // Get out immediately if no/improper arguments are found
        if (args.Length < 3)
        {
            return (int)Status.ImproperOrNoArgumentsFound;
        }

        try
        {
            string text;

            using (var streamReader = new StreamReader(args[0]))
            {
                text = streamReader.ReadToEnd();
            }

            text = Regex.Replace(text, args[1], args[2]);
            using (var streamWriter = new StreamWriter(args[0]))
            {
                streamWriter.Write(text);
            }
        }
        catch (Exception ex)
        {
            // text file manipulation failed
            exception = ex.ToString();
            status = Status.TextFileManipulationFailed;
        }

        try
        {
            WriteLog(args[0], args[1], args[2], exception, status);
        }
        catch
        {
            // log write failed
            if (status == Status.Success)
            {
                status = Status.LogWriteFailed; // if intStatus = 1 when we get here, no need to modify the value(its a complete failure)
                // if intStatus = 0 when we get here, make intStatus = 2 so as to clearly distinguish a "log write failed"
                // error from a "text file manipulation failed" error
            }
        }

        return (int)status;
    }

    // The new Logger for the exe
    private static void WriteLog(string arg0, string arg1, string arg2, string exception, Status status)
    {
        var logFileName = string.Format("D:\\TempLogs\\Grep-{0:yyyy-MM-dd_hh-mm-ss-tt}.log", DateTime.Now);

        using (var log = new StreamWriter(logFileName, true))
        {
            log.WriteLine("Argument 1: " + arg0);
            log.WriteLine("Argument 2: " + arg1);
            log.WriteLine("Argument 3: " + arg2);
            log.WriteLine("Status: " + status);
            log.WriteLine("Exception: " + exception);
        }
    }
}

Answer 5

Others have written in detail how you can improve your exception handling, which is really important, but your core logic can be reduced to the following:

private static void ReplaceInFile(string path, string pattern, string replacement)
{
    string contents = File.ReadAllText(path);
    string replaced = Regex.Replace(contents, pattern, replacement);
    File.WriteAllText(path, replaced);
}

A note on Performance

If you have a small number of relatively short files, your current approach should work. However, it's suboptimal for larger amounts of data as you need to execute your program once for each file, always keeping the entire file in memory.

However, you should not change to a more complicated approach unless you experience serious performance issues. In that case, you should explore File.ReadLines (which will only read a few lines into memory at once) and IEnumerable<T>.AsParallel (which allows you to process several files at once (you'd have to use temporary files to store the results).

Answer 6

Along with what E-Man, ANeves and Jesse have contributed I would suggest the following...

Put your arguments into variables at the top so the rest of the code is self-documenting. I recognize that you put comments at the top which describe the parameters, but variables names could better describe the information.

To make these more robust I would also do a bit more validation. I understand that exceptions will be thrown if the information is invalid but to me exceptions should only be thrown for unhandled problems, and invalid user input is something that should be handled.

Thirdly, if you want to catch exceptions when writing logs you might consider encapsulating that in the WriteLog method. Then you can return a bool indicating whether the log write was successful.

Putting it all together (including everyones suggestions). See what you think of this...

class Program
{
    private enum Status
    {
        Success,
        FileNotFound,
        ImproperOrNoArgumentsFound,
        TextFileManipulationFailed,
        LogWriteFailed
    }

    static int Main(string[] args)
    {
        Status status = Status.Success;
        string strException = string.Empty;

        //Get out immediately if no/improper arguments are found
        if (args.Length < 3)
        {
            return (int)Status.ImproperOrNoArgumentsFound;
        }

        string fileName = args[0];
        if (!File.Exists(fileName))
        {
            return (int)Status.FileNotFound;
        }

        string pattern = args[1];
        string replacementValue = args[2];

        try
        {
            using (StreamReader streamReader = new StreamReader(fileName))
            {
                string text = streamReader.ReadToEnd();
                text = Regex.Replace(text, pattern, replacementValue);
                using (StreamWriter streamWriter = new StreamWriter(streamReader))
                {
                    streamWriter.Write(text);
                }
            }
        }
        catch (Exception ex)
        {
            //text file manipulation failed
            strException = ex.ToString();
            status = Status.TextFileManipulationFailed;
        }

        if (!WriteLog(strException, status, fileName, pattern, replacementValue))
        {
            if (status == Status.Success)
            {
                status = Status.LogWriteFailed;
            }
        }

        return (int)status;
    }

    //The new Logger for the exe
    static bool WriteLog(string exception, Status status, params string[] arguments)
    {
        bool success = true;

        try
        {
            string logFile = string.Format("D:\\TempLogs\\Grep-{0:yyyy-MM-dd_hh-mm-ss-tt}.log", DateTime.Now);
            using (StreamWriter grepLog = new StreamWriter(logFile, true))
            {
                grepLog.WriteLine(string.Format("Status: {0} (0 for PASS, 1 for FAIL)", status));
                grepLog.WriteLine("Exception: " + exception);
                // TODO: Consider logging "no arguments" if that is the case.
                for (int i = 0; i < arguments.Length; i++)
                {
                    grepLog.WriteLine("Argument " + i + ": " + arguments[i]);
                }
            }
        }
        catch (Exception)
        {
            success = false;
        }

        return success;
    }
}

Answer 7

I searched extensively on stackoverflow and MSDN to comprehend all the technical stuff other community members pointed me to.

Specifically the following post was wonderful:

https://stackoverflow.com/questions/538060/proper-use-of-the-idisposable-interface

I am amazed at how such a small piece of code could have so much to learn from.

Gene S had given a code snippet which was beautifully done but had a small issue.

The following part of the code, I believe, has a small bug

try
        {
            using (StreamReader streamReader = new StreamReader(fileName))
            {
                string text = streamReader.ReadToEnd();
                text = Regex.Replace(text, pattern, replacementValue);
                using (StreamWriter streamWriter = new StreamWriter(fileName))
                {
                    streamWriter.Write(text);
                }
            }
        }

The StreamReader has StreamWriter inside it. The StreamReader has not released the input file when StreamWriter is trying to write to it. This causes a "File is in use by another process" error.

I have modified the code:

class Program
{
    private enum Status
    {
        Success,
        FileNotFound,
        ImproperOrNoArgumentsFound,
        TextFileManipulationFailed,
        LogWriteFailedButJobPASS,
        LogWriteFailedAndJobFAIL
    }

    static int Main(string[] args)
    {
        //--------------------------------------------------------
        //          args[0]=File to be manipulated
        //          args[1]=String to be replaced or PATTERN
        //          args[2]=String with which to replace args[1]
        //--------------------------------------------------------
        string fileName = args[0];
        string pattern = args[1];
        string replacementValue = args[2];

        Status status = Status.Success;
        string strException = string.Empty;

        //Get out immediately if no/improper arguments are found
        if (args.Length < 3)
        {
            status = Status.ImproperOrNoArgumentsFound;
            strException = "ImproperOrNoArgumentsFound";
            //Do not care if logging succeeded or not.
            //This is premature failing and return value is actually enough for installer.
            WriteLog(strException, status, fileName, pattern, replacementValue);
            return (int)status;
        }

        if (!File.Exists(fileName))
        {
            status = Status.FileNotFound;
            strException = "FileNotFound";
            //Do not care if logging succeeded or not.
            //This is premature failing and return value is actually enough for installer
            WriteLog(strException, status, fileName, pattern, replacementValue);
            return (int)status;
        }

        try
        {
            string text = string.Empty;
            using (StreamReader streamReader = new StreamReader(fileName))
            {
                text = streamReader.ReadToEnd();
                text = Regex.Replace(text, pattern, replacementValue);
            }

            using (StreamWriter streamWriter = new StreamWriter(fileName))
            {
                streamWriter.Write(text);
            }
        }
        catch (Exception ex)
        {
            //Text file manipulation failed
            strException = ex.ToString();
            status = Status.TextFileManipulationFailed;
        }

        //Really care about the success of logging.
        //What all happened have to be logged for installer troubleshooting etc...
        if (!WriteLog(strException, status, fileName, pattern, replacementValue))
        {
            if (status == Status.Success)
            {
                status = Status.LogWriteFailedButJobPASS;
            }
            else
            {
                status = Status.LogWriteFailedAndJobFAIL;
            }
        }
        return (int)status;
    }

    //The Logger for the exe
    static bool WriteLog(string exception, Status status, params string[] arguments)
    {
        bool success = true;

        try
        {
            string logFile = string.Format("D:\\TempLogs\\Grep.log", DateTime.Now);
            using (StreamWriter grepLog = new StreamWriter(logFile, true))
            {
                grepLog.WriteLine("---------------------------------------------------------------");
                grepLog.WriteLine(string.Format("{0:yyyy-MM-dd hh-mm-ss-tt}", DateTime.Now));
                for (int i = 0; i < arguments.Length; i++)
                {
                    grepLog.WriteLine("Argument " + i + ": " + arguments[i]);
                }
                grepLog.WriteLine(string.Format("Status: {0}", status));
                grepLog.WriteLine("Exception Stack: " + exception);
                grepLog.WriteLine("---------------------------------------------------------------");
            }
        }
        catch (Exception)
        {
            success = false;
        }

        return success;
    }

Answer 8

In OO programming every class has a single purpose. I would like to put file read and write in seperate class.

Here is how I would like to do this.

private static int Main(string[] args)
{
    FileManager fileAgent = new FileManager();
    Status status = Status.Success;
    string errMessage = string.Empty;

    if (args.Length < 3)
    {
        status = Status.ImproperOrNoArgumentsFound;
        fileAgent.Log(args, status, errMessage);
        return (int)status;
    }

    fileAgent.FileName = args[0];

    try
    {
        string text = fileAgent.ReadText();
        text = Regex.Replace(text, args[1], args[2]);
        fileAgent.WriteText(text);
    }
    catch (Exception ex)
    {
        status = Status.TextFileManipulationFailed;
        errMessage = ex.ToString();
    }

    fileAgent.Log(args, status, errMessage);
    return (int)status;
}