Read Lines From IIS Log

The purpose of this class is to read an IIS log (or multiple ones). One thing to note is that the columns can differ based on settings in IIS.

A couple of concerns that I have:

• Is The ILogReader Interface neccessary?

• Is the IFileReader the right way to allow me to stub out the File.ReadAllLines method?

• Any other refactoring / clean code suggestions

• How are my tests?

• Is the static variable a nice and clean way to setup test data?

public class LogReader : ILogReader
{
    private IFileReader _fileReader;
    private List _lines = new List();
    private string[] _infoLines = new[] { "#Version:", "#Date:", "#Software:", "#Fields:" };

    public LogReader(IFileReader fileReader)
    {
        _fileReader = fileReader;
    }

    public List LogEntries
    {
        get { 
            return _lines
                .Where(line => IsLogEntry(line))
                .Select(logEntryLine => new LogEntry(logEntryLine, Header))
                .ToList();
            }
    }

    private bool IsLogEntry(string[] line)
    {
        return !_infoLines.Contains(line.First());
    }

    public void AddFileToEntries(string fileName)
    {
        var newLines = _fileReader.ReadAllLines(fileName)
                        .Select(line => line.Split(' '))
                        .ToList();

        _lines.AddRange(newLines);
    }

    public List Header 
    { 
        get 
        {
            return _lines
                .FirstOrDefault(l => l.First()
                .Contains("#Fields"))
                .Skip(1)
                .Select(field => field.Replace("-",""))
                .ToList();
        }
    }
}

Here are my tests:

```
[TestClass]
public class LogReaderTests
{
private ILogReader _logReader;
Moq.Mock _fileReaderMock;
const string _logFileWith404String = "LogFileWith404";
const string _logFileWith200String = "LogFileWith200";

[TestInitialize]
public void Init()
{
_fileReaderMock = new Moq.Mock();

Let's take a look.


Is The ILogReader Interface neccessary?

It is not, as long as you reasonably expect that it will stay with just this one log reader. I think it is not out of the question that you might adapt your application to also account for different types of logs so I would leave it in there for future purposes.

It also indicates more that you are talking about a logreader instead of the logreader, which you'll need when adding more logs.


Is the IFileReader the right way to allow me to stub out the File.ReadAllLines method?

Yes, definitely. External dependencies should be stubbed out into their own layer so you have no interference with the actual system and thus stick to unit tests and not integration tests (they should live together, not replace eachother).

As a sidenote here: are you using an actual sample logfile or are you hardcoding the strings? I feel like you could benefit from adding an additional layer for mocking data in your unit tests. Read more here on that subject.


Any other refactoring / clean code suggestions

Honestly.. No. Your code is very clean and slim and it looks like you have little room for improvement, you're fine on that aspect.


How are my tests?

I prefer explicit private mentioning for variables. It leaves no room for confusion and it keeps your types aligned with eachother.

private ILogReader _logReader;
private Moq.Mock _fileReaderMock;
private const string _logFileWith404String = "LogFileWith404";
private const string _logFileWith200String = "LogFileWith200";

When it comes to unit testing method naming I very much appreciate the approach as described in The art of Unit Testing, 2nd Edition by Roy Osherove which comes down to

[UnitOfWorkName]_[ScenarioUnderTest]_[ExpectedBehaviour]

For example for your method named OpenLogAndReadHeaderEnsureThatFirstNonHeaderValueIsDate this could become

ReadingLogHeader_FirstNonHeaderValue_IsDate

Now you have clearly separated your method name's intentions and it's more easily to see where the problem is located when tests fail.

There are two methods (TestFirstValueCanConvertToDate and PrintAllLines) that don't test anything thus they shouldn't be marked with the TestMethodAttribute. Likewise they should be private if they're just helper methods (although I don't see the purpose of TestFirstValueCanConvertToDate).


Is the static variable a nice and clean way to setup test data?

I assume you are referring to the const variables considering I don't see any static behaviour aside from that.

No, I don't think they're necessary. It won't have any impact on your performance and you shouldn't have another class that tests the same class so it shouldn't be used for that purpose either.

That being said: it doesn't negatively impact you either so it is yours to choose if you keep them or not.

StackExchange Code Review Q#44429, answer score: 3