Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question

migrated from meta.codereview.stackexchange.com Nov 12 '12 at 16:05

This question came from our discussion, support, and feature requests site for peer programmer code reviews.

8 Answers 8

up vote 10 down vote accepted

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. http://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.

share|improve this answer
    
Hi ANeves, These are great tips. Many things have started making sense now. Also, do you mean to say that the code is good apart from these style pointers? –  msiyer Nov 12 '12 at 17:31
    
Yes, I do think it is a good start. There are lots of other things to improve, and many are addresses by the good suggestions of improvement in the thread; you will learn lots with this experience. :) –  ANeves Nov 13 '12 at 9:09

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
    }
    
share|improve this answer
    
Thank you E-Man. –  msiyer Nov 12 '12 at 17:59
    
Can you explain preferring string.Empty to "" ? I know StyleCop defaults to having that rule enabled, but there's lots of rules in StyleCop that seem more opinion than best practice. –  Andy Nov 12 '12 at 18:47
    
I think it's just that, a preference. As long as you are consistent, I don't think it matters. The reference to StyleCop was more or less a suggestion to help with style checking in general. My preference is for string.Empty because it's explicit, if a little wordy. That way "" isn't accidentally written as " " or some other similar mistake. –  E-Man Nov 12 '12 at 19:23
1  
Related: stackoverflow.com/questions/263191/… –  E-Man Nov 12 '12 at 19:30
2  
The reader of string.Empty can have a very high confidence that the writer meant 'the empty string'. The reader of "" might have a nagging suspicion that " " or " " or even "." was meant, but mistyped... –  AakashM Nov 13 '12 at 12:35

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.

share|improve this answer

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);
        }
    }
}
share|improve this answer
    
Thank you Jesse. I do a lot of stuff but am new to OO programming. Your help will ease my troubles. –  msiyer Nov 12 '12 at 18:00

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).

share|improve this answer
1  
+1 is not enough, this is a really useful answer. –  ANeves Nov 13 '12 at 9:12
    
I am still at the beginning of my exploration of C#. I will learn the stuff you mentioned and update the post. Thanks. –  msiyer Nov 17 '12 at 22:06

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;
    }
}
share|improve this answer
    
Hi Gene, thanks for replying. This being my first C# code, do you see this as a good starting? Just want to assess myself? –  msiyer Nov 12 '12 at 17:58
1  
It's a good starting point. The only big concern is not using the using clause. That could have caused problems without it. More importantly though is that you are reaching out to learn. In my view, that is what makes defines a good developer. –  Gene S Nov 12 '12 at 19:54
    
I think I found a small bug in the code given by you. I have added an answer to the chain. Have a look at it. –  msiyer Nov 13 '12 at 9:39
    
That is very possible since I wrote it in NotePad. Sorry, should have included that disclaimer in my answer. –  Gene S Nov 13 '12 at 14:56

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:

http://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;
    }
share|improve this answer
1  
Another possibility is look using the FileStream object, then you can pass that object into the StreamReader and StreamWriter. Or look at passing the StreamReader into the StreamWriter constructor to see if you can avoid the exception in that manner. There are several ways to achieve the same goal. Some might give you better performance then others. –  Gene S Nov 13 '12 at 15:06
1  
Will you ever be working with really huge files that won't fit into memory? If that is the case you may need to need read and write the file in sections instead of all at once. Of course, that will be much more difficult if your pattern can reach across words or lines of text. –  Gene S Nov 13 '12 at 15:10
    
No. My files are guaranteed to be small. But I have started to look into the big files scenario too. –  msiyer Nov 13 '12 at 16:10

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;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.