The first thing I notice, is inconsistency with naming.
Locals should be
camelCase, and should have meaningful names.
something aren't meaningful names.
Filename should be
Parameters are like locals - they should be
Filename should be
FileLines should be
Method/member names should be
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.
readInFile have no business being
Constant vs Read-Only
This variable has no business being a
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.
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] == '/')
else if (something[x][y] == '/' && something[x][y + 1] == '*')
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
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:
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++)
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.