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\$
  • \$\begingroup\$ Is it good enough? Without knowing the requirements, there's no way to answer that. \$\endgroup\$ – Mast Jun 25 '15 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\$ – AlanT Jun 25 '15 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\$ – Love Jun 25 '15 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\$ – AlanT Jun 25 '15 at 14:48
  • 1
    \$\begingroup\$ @Mat'sMug, it is a class libary. \$\endgroup\$ – Love Jun 25 '15 at 20:35
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\$
  • \$\begingroup\$ Path.Combine(filePath,fileName) is not an error. It is for the different target. Twice is okay. \$\endgroup\$ – Love Jun 25 '15 at 15:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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