Untested illogical code

A Junior Developer sent a Code Review for some C# code. There was a bug that if a user didn’t have permission for a particular record, the code would try and look for the record and couldn’t find it, then it would then crash.

Not only was the “fix” a classic case of merely hiding the bug, rather than addressing the fact of why the code was trying to find the record in the first place – but the fix produced couldn’t possibly work. The Junior Developer had written some code like this:

public boolean HasPermissionToAccess(int recordId)
{
 try
 {
    records.singleOrDefault(record => record.id == recordId);
    return true;
 }
 catch(InvalidOperationException)
 {
    return false;
 }
}

Another developer had beat me to the review, but he nicely asked questions to hint that this can’t possibly do anything beneficial; along the lines of “can you tell me where the exception is thrown?” and “can you tell explain why these return values work?”. I would have just said “you cannot possibly have tested this”.

The thing is, the Junior then gave an answer like “the singleOrDefault line will return null if the user doesn’t have permission”.

Yes that is correct Junior, but you ain’t doing anything with the return value. You are returning “true” regardless of what happens. i.e the user “HasPermissionToAccess”. Surely it should return “false”? Also how can the catch block ever execute? singleOrDefault will never throw that exception. Since his new method always returns true, then the calling code will still crash in the exact same place it did before.

I don’t understand how you can write code with the aim of fixing a crash, but it doesn’t even fix the crash. On a positive note, I guess it didn’t introduce any other issue, other than being completely redundant.