Method to count all comments in single external C# file

I recently had an interview question:


Write a method that counts all comments in a single external file.

It was a timed question and I wanted to know if this is the best way to accomplish the task. I'm also open to improvements, advice, etc.

(I didn't pass the interview.)

class Program
{
    static void Main(string[] args)
    {
        List FileLines = new List();
        const string Filename = @"file.txt";

        readInFile(Filename, FileLines);

        CountComments(FileLines);
    }

    public static void CountComments(List value)
    {
        int finalCount = 0;
        List something = value;
        bool inQuotes = false;

        for (int x = 0; x  FileLines)
    {
        using (StreamReader r = new StreamReader(Filename))
        {
            string Line;

            while ((Line = r.ReadLine()) != null)
            {
                FileLines.Add(Line);
            }
        }
    }
}

Naming

The first thing I notice, is inconsistency with naming.

Locals

• FileLines

• Filename

• finalCount

• something

• inQuotes

• x

• y

• r

• Line

Locals should be camelCase, and should have meaningful names. x, y, r and something aren't meaningful names. Filename should be fileName.

Parameters

• args

• value

• Filename

• FileLines

Parameters are like locals - they should be camelCase. Filename should be fileName, and FileLines should be fileLines.

Methods

• Main

• CountComments

• readInFile

Method/member names should be PascalCase. readInFile should be ReadInFile - and that's not exactly a great name either. You're reading the file into a List ...that's passed as a parameter. Not the most intuitive approach.

Accessibility

Methods CountComments and readInFile have no business being public.

Constant vs Read-Only

This variable has no business being a const:

const string Filename = @"file.txt";

It should be an instance-level private static readonly string field, because a const should be strictly for something that has zero chance of ever changing in a future version. A file path is definitely something that can change, declaring it as a const is a semantic mistake.

Approach

readInFile should have a return type instead of having the side-effect of adding items to a List parameter. While legal, a better way to convey that a method will have side-effects on a parameter, is to ask for an out parameter, or to pass the list by reference using the ref keyword. Your method takes its output through its input channel, and that doesn't feel right at all.

I would have gone with File.ReadAllLines instead, which returns a string[] array where each element is a line in the specified file, or better (given C# 4+), File.ReadLines, which returns an IEnumerable instead: this means the whole readInFile method could have been inlined, and the FileName just specified as a hard-coded parameter value (or read from the args command-line arguments, if you wanted to get fancy).

As for the actual commend-finding logic...

if (something[x][y] == '/' && something[x][y + 1] == '/')
{
    finalCount++;
    break;
}
else if (something[x][y] == '/' && something[x][y + 1] == '*')
{
    finalCount++;
    break;
}

Can you spot the smell? Why do you need two branches, if both are going to do exactly the same thing?

I would have expected CountComments to do just that: count the comments. Yours is breaking SRP by also performing output - the method should've had an int return type, and just returned the result, leaving it up to the caller to decide what to do with it.

This one puzzles me:

List something = value;

Why not work off value? Why introduce a new meaningless identifier (something? really?) to make a copy of another meaningless identifier (value would already be better as values, but lines or even content would have been sooooo much better!).

I like that you stop iterating characters in a line after you've found a comment.

But instead of nested loops, I would have written a function that takes a string and returns a bool, to encapsulate the logic that basically says "is there a comment anywhere in that string?" - assuming C# 3.5+, the whole method could have looked like this:

return lines.Count(HasComment);

If you were constrained to 2.0, you would've had to perform the loop explicitly - still (assuming a string[] lines parameter):

int result = 0;
for (int line = 0; line < lines.Length; line++)
{
    if (HasComment(lines[line]))
    {
        result++;
    }
}
return result;

And then depending on the time remaining you could have refined the HasComment logic, for example to skip lines that are inside a multiline comment.

StackExchange Code Review Q#97892, answer score: 15