Code critique

It's well understood that developers, and especially line-of-business developers, spend much more time reading and understanding existing code than writing new code. Yet one of the most common interview approaches emphasises writing new code.

So one of my standard developer interview questions is to give the candidate a few lines of rather dodgy code and ask the following:

• Explain what the code is doing.
• Explain any significant issues with the code.
• Explain how to prioritise the issues.
• Explain how to fix the issues.

Below is a relatively simple example of fabricated C# code that has many issues. The context is deliberately left ambiguous in the hope that the candidate will ask some questions to clarify. The resulting discussion usually leads to some insights into the candidate's level of maturity when approaching code written by another developer.

For reference, here are my comments on the code fragment above:

• Why on earth is this code using a text file rather than a database to store login credentials?
• Why is there no parameter validation, e.g. for null, maximum length, legal path, etc? Can the candidate justify his/her decisions about what parameter validation should be done?
• Why is there no check for an event listener? Can the candidate does this in a thread-safe way?  Is adding an anonymous delegate to the event sufficient?  What other race condition exists for this event?
• Why is there no OnUserFound method, the normal pattern for raising events?  How does the candidate propose to create this method? Is it protected and virtual, to allow more flexible overriding of the base event?
• The FileExists check has a race condition   - the file could be deleted, locked, moved, or have its permissions changed between the check and the open. How does the candidate propose to fix this?
• The StreamReader could leak . How does the candidate propose to fix this?
• The code catches System.Exception. Why is this normally a bad idea?  How should exception management be fixed in this instance? Why should Try...Finally be much more common than Try...Catch?  
• It looks like the password is being stored as plain text. Why is this a security hole? How does the candidate propose to fix this?
• The IsLoginCorrect method isn't thread-safe in general. Is that important? How should it be fixed?
• The IsLoginCorrect method has no comments. What type of code comments (e.g. context, why, etc) does the candidate suggest? Can the candidate give some good and bad code comment examples? Is this a good comment?