# Configuring NLog

I have this method:

public void AddNLogConfigurationTypeTagret()
{
var filePath = _loggerModel.file_path_pattern.Replace("\\YYYY\\MM\\DD", "");
var dateTime = DateTime.Now;
filePath += "\\" + dateTime.Year + "\\" + dateTime.Month.ToString() + "\\" + dateTime.Day;
var filePattern = _loggerModel.file_name_pattern.Split('.');
var dateTimeFormat = filePattern[1].Replace("{", "").Replace("}", "").ToString();
var fileName = filePattern[0] + '.' + dateTime.ToString(dateTimeFormat) + "." + filePattern[2];

var fileTargetWithStackTrace = new FileTarget();
fileTargetWithStackTrace.Layout = _loggerModel.layout + "|${stacktrace}"; fileTargetWithStackTrace.Name = FILE_WITH_STACK_TRACE; fileTargetWithStackTrace.FileName = Path.Combine(filePath, fileName); _nLogLoggingConfiguration.AddTarget(FILE_WITH_STACK_TRACE, fileTargetWithStackTrace); var fileTargetWithoutStacktrace = new FileTarget(); fileTargetWithoutStacktrace.Name = FILE_WITHOUT_STACK_TRACE; fileTargetWithoutStacktrace.Layout = _loggerModel.layout; fileTargetWithoutStacktrace.FileName = Path.Combine(filePath, fileName); _nLogLoggingConfiguration.AddTarget(FILE_WITHOUT_STACK_TRACE, fileTargetWithoutStacktrace); }  It seems that there are similarity inside the code, so I refactored it: public void AddNLogConfigurationTypeTagret() { var filePath = _loggerModel.file_path_pattern.Replace("\\YYYY\\MM\\DD", ""); var dateTime = DateTime.Now; filePath += "\\" + dateTime.Year + "\\" + dateTime.Month.ToString() + "\\" + dateTime.Day; var filePattern = _loggerModel.file_name_pattern.Split('.'); var dateTimeFormat = filePattern[1].Replace("{", "").Replace("}", "").ToString(); var fileName = filePattern[0] + '.' + dateTime.ToString(dateTimeFormat) + "." + filePattern[2]; var fileTargetWithStackTrace = new FileTarget(); fileTargetWithStackTrace.Layout = _loggerModel.layout + "|${stacktrace}";
fileTargetWithStackTrace.Name = FILE_WITH_STACK_TRACE;
fileTargetWithStackTrace.FileName = Path.Combine(filePath, fileName);

var fileTargetWithoutStacktrace = new FileTarget();
fileTargetWithoutStacktrace.Name = FILE_WITHOUT_STACK_TRACE;
fileTargetWithoutStacktrace.Layout = _loggerModel.layout;
fileTargetWithoutStacktrace.FileName = Path.Combine(filePath, fileName);
}

{
}


Is it good enough?

Edit

By the comment, I made some changes.

 public void AddNLogConfigurationTypeTagret()
{
var dateTime = DateTime.Now;
String.Format("\\yyyy\\MM\\dd", dateTime);
var filePath = _loggerModel.file_path_pattern.Replace("\\YYYY\\MM\\DD", "") + dateTime.ToString("\\\\yyyy\\\\MM\\\\dd", CultureInfo.InvariantCulture);
var filePattern = _loggerModel.file_name_pattern.Split('.');
var dateTimeFormat = filePattern[1].Replace("{", "").Replace("}", "").ToString();
var fileName = filePattern[0] + '.' + dateTime.ToString(dateTimeFormat, CultureInfo.InvariantCulture) + "." + filePattern[2];

var fileTargetWithStackTrace = new FileTarget();
fileTargetWithStackTrace.Layout = _loggerModel.layout + "|\${stacktrace}";
fileTargetWithStackTrace.Name = FILE_WITH_STACK_TRACE;
fileTargetWithStackTrace.FileName = Path.Combine(filePath, fileName);

var fileTargetWithoutStacktrace = new FileTarget();
fileTargetWithoutStacktrace.Name = FILE_WITHOUT_STACK_TRACE;
fileTargetWithoutStacktrace.Layout = _loggerModel.layout;
fileTargetWithoutStacktrace.FileName = Path.Combine(filePath, fileName);
}

{
}

• Is it good enough? Without knowing the requirements, there's no way to answer that.
– Mast
Jun 25 '15 at 14:25
• Why are you refactoring? What do you hope to achieve? Have you achieved this with the change?@Mast is correct, in that without knowing your goals we cannot tell if you have met them, but I feel that the change has not achieved any of the goals of refactoring link Jun 25 '15 at 14:31
• @AlanT, the code itself was working. But I was told since fileTargetWithStackTrace and fileTargetWithoutStacktrace part are very similar. So we can combine them together with the clean code principle to reduce the length. The thing is I don't see the obvious change.
– Love
Jun 25 '15 at 14:36
• @Love There is a lot of repetitive coding in the method. Refactoring it to remove the repetition is a good plan. Looking at the new version though, I don't see that any has been removed. All that has changed is wrapping a single line call with a method and then calling it twice. Net addition to the code AddFileTarget. Net reduction in original method - none. It might be worth looking at moving all the lines in each block out to the new method. Jun 25 '15 at 14:48
• @Mat'sMug, it is a class libary.
– Love
Jun 25 '15 at 20:35

Some quick remarks:

• I'm pretty sure filePath += "\\" + dateTime.Year + "\\" + dateTime.Month.ToString() + "\\" + dateTime.Day; can be replaced by dateTime.ToString() with an appropriate parameter.

• Also, why are you doing a replace on _loggerModel.file_path_pattern when assigning its value to filePath and then concatenate a value to filePath? Do this in one go.

• I would use string.Format instead of concatenation to create the filename.

• You twice execute Path.Combine(filePath, fileName);. Is that an error? Because right now it looks like you're writing to the same file each time.

• The creation of a FileTarget and the assignment of its Layout, Name and FileName should move to AddFileTarget (you'll need to change the parameters of that method, of course).

• FILE_WITH_STACK_TRACE, FILE_WITHOUT_STACK_TRACE: constants should be PascalCase.

• Path.Combine(filePath,fileName) is not an error. It is for the different target. Twice is okay.
– Love
Jun 25 '15 at 15:13