4
\$\begingroup\$

I have started to read Robert C. Martin Clean Code book.

To learn and gain more expreiences I wrote a single Log class. So I want some suggestions to improve this code.. This is a part of the Log class:

public class FileLogger : ILog
{
    private string directoryPath = string.Empty;
    private string fileName = string.Empty;

    public FileLogger(string logDirectoryPath, string logFileName)
    {
        if (string.IsNullOrEmpty(logDirectoryPath))
        {
            throw new ArgumentException(nameof(logDirectoryPath), "Write some error message...");
        }

        if (string.IsNullOrEmpty(logFileName))
        {
            throw new ArgumentException(nameof(logFileName), "Write some error message...");
        }

        this.directoryPath = logDirectoryPath;
        this.fileName = logFileName;
    }

    public void LogMessage(string message)
    {
        try
        {
            if (!string.IsNullOrEmpty(message))
            {
                if (!LogDirectoryExists(directoryPath))
                {
                    CreateLogDirectory(directoryPath);
                }

                WriteToLogFile(message);
            }
        }
        catch (Exception ex)
        {
            //Catch exception, do something...
        }
    }

    private bool LogDirectoryExists(string logDirectoryPath)
    {
        return Directory.Exists(logDirectoryPath);
    }

    private void CreateLogDirectory(string logDirectoryPath)
    {
        Directory.CreateDirectory(logDirectoryPath);
    }

    private void WriteToLogFile(string message)
    {
        using (var logFile = new FileStream(Path.Combine(directoryPath,fileName), FileMode.Append, FileAccess.Write))
        {
            using (var logFileWriter = new StreamWriter(logFile))
            {
                logFileWriter.Write(message);
            }
        }
    }
}

I wrote 3 different function to write a message to a log. I try to use SRP and Command Query Separation, but I don't sure this is a good in this way. Maybe there are another problems with this code, example naming or variable declarations.

\$\endgroup\$
  • \$\begingroup\$ Use Path.Combine instead of manual path parts concatenation with backslash. Also there is no necessity to specify types of variable explicitly in your usings, you can use var. Field names (DirectoryPath, FileName) shouldn't start with uppercase letter. And check message on null in LogMessage. \$\endgroup\$ – Maxim Jul 14 '17 at 9:21
  • \$\begingroup\$ Thanks. To check message on null, if I think right I had to write a function for this and calls this in LogMessage, due to SRP? Sorry if I ask "small things", but I want to understand this well. \$\endgroup\$ – NewbieUser Jul 14 '17 at 9:36
  • \$\begingroup\$ You can check an argument directly in LogMessage, it is absolutely OK. Also check logDirectoryPath and logFileName in constructor. \$\endgroup\$ – Maxim Jul 14 '17 at 10:33
  • \$\begingroup\$ Based on your suggestions , I rewrite. \$\endgroup\$ – NewbieUser Jul 14 '17 at 11:44
  • 1
    \$\begingroup\$ @Maxim please write an answer. Comments are not for reviewing code \$\endgroup\$ – t3chb0t Jul 14 '17 at 13:19
2
\$\begingroup\$

After discussion in comments your code became much better.

Also if directoryPath and fileName will not be changed after initialization in constructor you can define these fields as readonly:

private readonly string directoryPath = string.Empty;
private readonly string fileName = string.Empty;

This code

using (var logFile = new FileStream(Path.Combine(directoryPath,fileName), FileMode.Append, FileAccess.Write))
{
    using (var logFileWriter = new StreamWriter(logFile))
    {
        logFileWriter.Write(message);
    }
}

can be replaced with

File.AppendAllText(Path.Combine(directoryPath, fileName), message);

Also ArgumentException's constructor takes message as first argument and paramName as second, so you need to call it like this:

throw new ArgumentException("Write some error message...", nameof(logDirectoryPath));
\$\endgroup\$
  • 1
    \$\begingroup\$ Because the constructor initializes those fields, they shouldn't be initialized to string.Empty either. It's redundant. \$\endgroup\$ – Jesse C. Slicer Jul 14 '17 at 19:23

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.