Hmmm, taking a while guess, I assume if you have a 0 byte file then it would attempt to create it and would throw since it already exists?

file is string which contains filename. How can you decide from length of file name number of pages ? A.

Creating the file is not exception-safe, there is a race between creating it and opening it (not sure whether that one matters). I'd make this into a single new FileStream call with different flags as input.

var fileInfo = new FileInfo(file);

this line will throw an expection if the file text is empty

Also, creating over a zero-byte file will do that without FileShare.Read.

This call will throw exception: MemoryMapPager("")

Wild guess: theoretically, there is a tiny interval between fileInfo.Exists and fileInfo.Create statements, some other process/thread could create and open this file during this interval (so e.g. exception about already opened file could be thrown). It reminds me this answer on SO (but in python): http://stackoverflow.com/a/85237/543205

Considering the following line:

_allocatedPages = file.Length / PageSize;

file.Length is the length of the path to the file, not the length of the file itself.

problem I see is that if Open fails then you have allocatedPages set an you should not(since is class field), also you don't need that much code, to avoid it I would do it like this:

public MemoryMapPager(string file,FlushMode flushMode.Full) { fileStream = new FileStream(filePath, FileMode.OpenOrCreate, FileAccess.ReadWrite);

_allocatedPages=fileStream.Length/PageSize;

}

Wasn't the second answer (@Ales) correct? Condition should say:

if (fileInfo.Exists == false || fileInfo.Length == 0)

I read FileStream and FileInfo code first looking for something tricky and then noticed this :) Unit tests would catch it - compiler had no chance.

To expand on @Ales's answer you should used fileInfo.Length not file.Length.

In case file exists, fileInfo does not get closed? Soooo, did I get the job?

Don't know the real context, but assume it should be something like this:

if (fileInfo.Exists == false) { fileInfo.Create().Close(); } _allocatedPages = fileInfo.Length / PageSize;

_allocatedPages=fileStream.Length/PageSize + (fileStream.Length%PageSize>0?1:0);

Yep, both instances of file.Length should be fileInfo.Length. You might also have an issue if the directory doesn't exist, maybe add fileInfo.Directory.Create() before creating the file.

This line looks strange _allocatedPages = (file.Length / PageSize); it should be +1

Cheers

That's why I avoid calling "file" a variable that contains a path...

Haha @Ales - nice catch.

The code should be like this: _allocatedPages = Math.Ceiling(fileInfo.Length / (double) PageSize);

So if the file length is 20 and the size of page is 17 then the number of allocated pages should be 2.

would it be evil? You'd get to see how the candidate approached the problem, Ales spotted the major "typo that compiles" problem, but an emergent discussion about race conditions, divide by zero etc etc would make for a nice ice breaker.

Obviously, file might be null or empty (throw ArgumentException), and checking for the length of the filename is wrong. However, do we know that PageSize has been initialized with a non-zero value (division by zero)? Additionally, why check if the file exists, possibly create (and close) and then open it again? One could arguably just use FileMode.OpenOrCreate and use _fileStream.Length to calculate _allocatedPages. That would get rid of race conditions and some code.

"string file" -- poor variable naming caused this bug.

The method signature is missing a return type.

@Michael, it's a ctor.

So I'm not sure if it's a good idea to create/open file in ctor.

The obvious bug aside, isn't it a bad idea to do IO things like creating and opening files in constructors?

file.Length is used in a couple of places where I'm assuming you are not interested in the length of the file path. :)

Speaking of interviews - watching someone review unfamiliar code like this is orders of magnitude more useful than FizzBuzz, and way less stressful for the applicant, IMO.

fileinfo.Open will throw if the file did not exist in the first place. I just ran into this problem. it's the difference between System.IO.File and System.IO.FileInfo

I would rewrite it like this: <script src="https://gist.github.com/alexandrnikitin/6719301.js"></script>

So I'm not sure if it's a good idea to create/open file in ctor.

The obvious bug aside, isn't it a bad idea to do IO things like creating and opening files in constructors?

I think that should be okay if that is the documented and expected behavious, not an unexpected side-effect.

fileinfo.Open will throw if the file did not exist in the first place. I just ran into this problem. it's the difference between System.IO.File and System.IO.FileInfo

Well, that should depend on the FileMode parameter since all that Open does is construct a new FileStream.

Ups, sorry, can't paste gists. Here's code:

public MemoryMapPager(string fileName, FlushMode flushMode = FlushMode.Full) { _flushMode = flushMode;

var fileInfo = new FileInfo(fileName);
var fileLength = fileInfo.Length;
var isFileEmpty = fileLength == 0 || fileInfo.Exists == false;

_fileStream = fileInfo.Open(FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.Read);

_allocatedPages = isFileEmpty ? 0 : (long) Math.Ceiling((float) fileLength/PageSize);

}

• Why re-create file on if "file.Length==0" case? _allocatedPages = (file.Length / PageSize) need to check PageSize must greather than zero and MemoryMapPager need to implement IDisposable interface for disposing FileStream

Some AV software scan on file create after the file handle is closed, on an enough heavily-loaded machine, you will get an UnauthorizedAccessException once in a blue moon in the fileInfo.Open line.

Fixing it is as simple as touching the file only once (as well as the typos).

You use file.Length instead of fileInfo.Length.

Also, _fileStream isn't being initialized in all cases. This means in case the file doesn't exist or (after fixing the name of the file variable and changing the length check to that of the FileInfo), you may get NullReferenceException's trying to use the instance you just created.

If the intention is to not allow a MemoryMapPager to be constructed, an InvalidOperationException or similar should be thrown to catch the problem earlier on.

The class name is also misleading, the name "Pager" implies that this is concerned only with the reading and paging of things. I'm not an expert on memory mapped views but it seems weird to me that it should care about FlushMode.

If the class will be used like a regular memory mapped view (sliding window over a memory space with read/write access), it should be named better. Perhaps, MemoryMappedView ? or if it is dealing with memory view the size of memory pages or disk-pages, then MemoryMapPage is better (without the "r").

Quite a good argument in favour of a TDD approach, this one.

I think the important code defect is that you've named the first parameter "file" and not something like "fileName".

Change file.Length to fileInfo.Length, the length of a file's is different from the length of a string named file.

Oren, the bugs are pretty obvious, but anyway we're waiting for your comments :)