Method to count all comments in single external C# file

Question

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<string> FileLines = new List<string>();
        const string Filename = @"file.txt";

        readInFile(Filename, FileLines);

        CountComments(FileLines);
    }

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

        for (int x = 0; x < something.Count; x++)
        {
            if (something[x] != @"""")
            {
                inQuotes = !inQuotes;
            }
            for (int y = 0; y < something[x].Length; y++)
            {
                if (something[x][y] == '/' && something[x][y + 1] == '/')
                {
                    finalCount++;
                    break;
                }
                else if (something[x][y] == '/' && something[x][y + 1] == '*')
                {
                    finalCount++;
                    break;
                }
            }
        }
        Console.WriteLine("Total number of comments is " + finalCount);


    }

    public static void readInFile(string Filename, List<string> FileLines)
    {
        using (StreamReader r = new StreamReader(Filename))
        {
            string Line;

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

Answer 1

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<string> ...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<string> 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<string> 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<string> 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.

Answer 2

Others have already mentioned style and the broken quote detection. But there's another bug. Take this example:

int a /* foo */ = /* bar */ 1; // baz

How many comments would you say this snippet contains? How many comments does your code think it contains?

You're assuming there's only a single comment per line. That is wrong.

What you need to be doing is ignoring anything between /* and */, and ignoring anything from // to the end of the line. Everything else should be checked. You can't skip the line when you detect the start of a delimited comment.

---

Also, did the original question specify what should be considered a single comment? How many comments here?

// The quick brown fox
// jumped over the lazy dog.

As a human, you might call that a single comment.

What about here?

/// <summary>
/// Foo class
/// </summary>

That's technically a single doc-comment.

Answer 3

First, use LINQ:

foreach (var line in fileLines.Where(l => l.Contains("//") || l.Contains("/*")))
{
    // Check if comment is in a comment or in a quote
}

---

It appears you have a bug with the quote validation. This variable only appears three times:

bool inQuotes = false;

That is the first. The other two are here:

if (something[x] != @"""")
{
    inQuotes = !inQuotes;
}

Either you can remove this variable, or there is a bug in your program.

---

CountComments() has no business printing anything:

Console.WriteLine("Total number of comments is " + finalCount);

Your method signature should be an int, which you can print if you wish.

---

Because the sole responsibility of readInFile() is to create a list of strings, I would pass that parameter as an out parameter to signify this. Then, instead of adding the values to the passed list, you create the list with the values and assign the new list to the passed list.

Answer 4

What if a line ends with a single /? Your code will attempt to read past the end of the string, with y + 1.