HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpModerate

Replace strings in a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filestringsreplace

Problem

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 str

Solution

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.

Code Snippets

//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();
}

Context

StackExchange Code Review Q#18501, answer score: 10

Revisions (0)

No revisions yet.