1
\$\begingroup\$

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);
    AddFileTarget(fileTargetWithStackTrace);

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

private void AddFileTarget(FileTarget fileTarget)
{
    _nLogLoggingConfiguration.AddTarget(fileTarget.Name, fileTarget);
}

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);
        AddFileTarget(fileTargetWithStackTrace);

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

    private void AddFileTarget(FileTarget fileTarget)
    {
        _nLogLoggingConfiguration.AddTarget(fileTarget.Name, fileTarget);
    }
\$\endgroup\$
7
  • \$\begingroup\$ Is it good enough? Without knowing the requirements, there's no way to answer that. \$\endgroup\$ Commented Jun 25, 2015 at 14:25
  • \$\begingroup\$ 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 \$\endgroup\$ Commented Jun 25, 2015 at 14:31
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Jun 25, 2015 at 14:36
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Commented Jun 25, 2015 at 14:48
  • 1
    \$\begingroup\$ @Mat'sMug, it is a class libary. \$\endgroup\$ Commented Jun 25, 2015 at 20:35

1 Answer 1

Reset to default
2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Path.Combine(filePath,fileName) is not an error. It is for the different target. Twice is okay. \$\endgroup\$ Commented Jun 25, 2015 at 15:13

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.