Code through the looking glassFinding today's log file
You might have noticed that we are looking for more people again. Mostly because I gripe about some of the bad ones here every now and then.
One of my absolute requirements is that I want to read a candidate's code before hiring them. Some of them have made significant contributions to Open Source code, but a large number don't have any significant body of code that they can show. That is why we ask candidates to send us a coding test.
We had people flat out refuse to do the coding test, and some who thought that the questions were incredibly hard and unrealistic. We had people who sent in code that was so bad it caused migraines, we had people who sent in code that wouldn't compile, we had people do stuff like read a 15TB file (16,492,674,416,640)! times. And just to clarify, that is factorial(16492674416640). I don't know what the actual value of this number is, but is it big.
The nice thing is that you can usually tell right away when the code is bad. We also play a game called "guess the candidate's background". We have about 89% success rate there*.
Spotting good code is much harder, because a really successful submission is going to be boring. It does what needs to be done, and that is it. Our most recent hire's code submission was so boring, we had to analyze it using our standard metrics to find issues (our standard metrics are for production code running in a high performance DB for months on end, not the same metrics we evaluate code submissions).
And then… then there is this candidate, whose code is so unique that I decided to dedicate a full week to explore it. The code looks good, documented, there are explanations that show what is going on and they are going in the right direction, on the surface of it.
And then there is the devil in the details. I have quite a lot to write about this particular submission, but I'll just start with the following:
//Find today's log file in the directory public string LogFileFinder() { string[] files = Directory.GetFiles(LoggingAggregation.Program.GlobalVar.path, "*.log"); for (int i = 0; i < files.Length; i++) { int slash = files[i].LastIndexOf(@"\"); int dot = files[i].LastIndexOf("."); string fileName = files[i].Substring(slash + 1, dot - slash - 1); string fileDate = DateTime.Now.ToString("yyyy-MM-dd"); if (fileName.Equals(fileDate)) return fileName + ".log"; } return null; }
Now, another way to write this code would be:
Path.Combine(LoggingAggregation.Program.GlobalVar.path, DateTime.Now.ToString("yyyy-MM-dd") +".log")
I literally stared at this piece of code for several minutes, trying to figure out what is actually going on there. I wasn't able to.
As an aside, we sent the candidate a rejection notice, along with a few pointers to some of the more egregious things that were wrong in the code. They know how to code, it is just that it goes sideway in the middle.
* The game call to guess the candidate's default language settings. That is, someone who write C# code, but has been brought up on C, so have a C style to the code.
More posts in "Code through the looking glass" series:
- (18 Mar 2016) And a linear search to rule them
- (17 Mar 2016) Sorting large data sets
- (16 Mar 2016) I'm the God of all OOP
- (15 Mar 2016) All you need is a dictionary (and O(N) )
- (14 Mar 2016) Finding today's log file
Comments
a candidate who doesn't know Path.Combine / Path.GetFileName , is definitely isn't coming from .net background. this is so obvious for the basic working with files. I think your requirements should be .net experience as a must.
@Uri, I would say further that they don't know the common Path functions because they don't know how to use google, which would have (most probably - I didn't check) pointed them to a definitive stackoverflow answer. I don't expect normal people to be able to use MSDN, but google...? This code also looks VB6-ish.
This code finds an existing log file with the current date if it exists. It does not simply return a path to a new log file. It's pretty easy to read and the name of the function makes it clear that it's finding a log file rather than generating a path. I don't know what the requirements were but the code doesn't seem insane to me.
Trevor
The code is broken. The simplest 'working' implementation is
public string LogFileFinder() { // note: should check for invalid path characters before calling path.combine var expectedPath = Path.Combine(LoggingAggregation.Program.GlobalVar.path, DateTime.Now.ToString("yyyy-MM-dd") +".log") ;
// note: will always return false if expectedPath.Length > 260 characters // note: behavior is undefined if expectedPath is also the name of a directory if (File.Exists(expectedPath)) return expectedPath;
}
Specifically, there are at least 3 issues in the submitted code 1: Directory.GetFiles(LoggingAggregation.Program.GlobalVar.path, "*.log"); (note: will also toss an exception if the first parameter is a filename) Better hope log directory cleanup is on! Should at least have used the IEnumerable version
2: string fileDate = DateTime.Now.ToString("yyyy-MM-dd"); if (fileName.Equals(fileDate)) return fileName + ".log";
treats "2016-03-14.log" and "2016-03-14.myapp.log" as the same file, and in the second case will return the wrong filename
3: return fileName + ".log"; loses path information. So any attempt to write to this file will by default write to Environment.CurrentDirectory
Ayende
comment formatting guide? it isn't markdown
comment preview does not show the comment as it will actually appear
It would be great to see some of the accepted, "boring" submissions too
@Stephen, you are just grasping at straws trying to justify Orens high standards. None of the issues you are mentioning exist. 1. If value of constant is invalid then there is no solution. Other code is not going to work either. 2. This is just not true. After seeing this I think you shouldn't be criticizing anyone. 3. Why are you assuming what the method output should be?
Oren knows what kind of people here likes on hours team, and as someone already said, this code just means person is not coming from C# background. But they are not dumb, solution is not wrong, it does not tell us whether person is capable of learning s quickly. So I don't understand what was the point of putting this code on display.
Uri, A candidate that doesn't know that they should do some form of File.Exists? Or doesn't know that iterating over the whole directory is not efficient?
Trevor, See Stephen's comments on the code. The most problematic issue is that it is going to iterate over all of the files, and then do a bunch of manual string operations and allocations on the resulting files. It is a LOT more complex than it needs to be, and it is doing a lot more useless work
Nikola, The issue isn't the background. I have had people with very strong C++ background, complete with m_ prefixes and you_there naming conventions. But the key here is that an operation that should be O(1) (File.Exists and that is pretty much it) is an O(N) operation
@Oren, true, purpose of the method itself evaded me, focused too much on the content. If he had real knowledge of the framework, who knows how his trail of thoughts would lead him to solution. Still there is one situation where his code would work, but File.Exists would not. Application will return the path if it really exists, and will not try to probe a file name. In case applications running user does not have read permission for the file, but has list permission for containing folder. But this is now my grasping at straws, this was probably not the situation he was in.
Comment preview