Hi

I wanted to show you a simple file loader class that can read both Asynchronously and Synchronously and have you let me know if its "good and clean".

Also, could the buffer be public or should I have some method that returns a pointer to the vector or something for when I want to process it later form outside?

class FileLoader
{
public:
	FileLoader(std::string Filename, bool Asynchronous);
	~FileLoader();

	DWORD GetStatus();

private:
	void Tick();
	void ReadAsynch();
	void ReadSynch();

	HANDLE file;
	DWORD Size;
	DWORD Status;
	DWORD NumberOfBytesTransfered;
	OVERLAPPED OverlappedInfo;
	std::vector<BYTE> buffer;
};

FileLoader::FileLoader(std::string Filename, bool Asynchronous)
{
	Size = 0;
	Status = 0;
	NumberOfBytesTransfered = 0;
	ZeroMemory(&OverlappedInfo, sizeof(OverlappedInfo));

	if (Asynchronous)
	{
		file = CreateFileA(Filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN | FILE_FLAG_OVERLAPPED, NULL);
	}
	else {
		file = CreateFileA(Filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN, NULL);
	}

	if (file == INVALID_HANDLE_VALUE)
	{
		Status = FileLoaderErrorInvalidHandle;
	}
	else {
		Size = GetFileSize(file, NULL);
		if (Size == INVALID_FILE_SIZE)
		{
			Status = FileLoaderErrorInvalidSize;
			CloseHandle(file);
		}
		else {
			buffer.resize(Size);

			if (Asynchronous)
			{
				ReadAsynch();
			}
			else {
				ReadSynch();
			}

		}
	}
}

FileLoader::~FileLoader()
{
	CloseHandle(file);
}

DWORD FileLoader::GetStatus()
{
	Tick();
	return Status;
}

void FileLoader::ReadAsynch()
{
	OverlappedInfo.hEvent = CreateEvent(NULL, true, false, NULL);

	if (ReadFile(file, &buffer[0], Size, NULL, &OverlappedInfo))
	{
		// Operation has completed immediately.
		Status = FileLoaderComplete;
	}
	else {
		if (GetLastError() != ERROR_IO_PENDING)
		{
			// Some other error occurred while reading the file.
			CloseHandle(file);
			buffer.clear();
			Status = FileLoaderErrorX;
		}
		else {
			//Operation has been queued and will complete in the future. 
			Status = FileLoaderInTransfer;
		}

	}
}

void FileLoader::ReadSynch()
{
	if (ReadFile(file, &buffer[0], Size, &NumberOfBytesTransfered, NULL))
	{
		// Operation has completed.
		if (Size == NumberOfBytesTransfered){
			Status = FileLoaderComplete;
		}
		else {
			Status = FileLoaderErrorReadSize;
		}
	}
	else {
		// Some other error occurred while reading the file.
		CloseHandle(file);
		buffer.clear();
		Status = FileLoaderErrorX;
	}
}

void FileLoader::Tick()
{
	if (Status == FileLoaderInTransfer)
	{
		if (HasOverlappedIoCompleted(&OverlappedInfo))
		{
			if (GetOverlappedResult(file, &OverlappedInfo, &NumberOfBytesTransfered, false) != 0)
			{
				// Operation has completed.
				CloseHandle(file);
				if (Size == NumberOfBytesTransfered){
					Status = FileLoaderComplete;
				} else {
					Status = FileLoaderErrorReadSize;
				}

			}
		}
	}
}

//FileLoader defs
#define	FileLoaderUnknown		0
#define	FileLoaderComplete		1
#define	FileLoaderInTransfer		2
#define	FileLoaderErrorInvalidHandle	10
#define	FileLoaderErrorInvalidSize	11
#define	FileLoaderErrorX		12
#define	FileLoaderErrorReadSize		13
#define	FileLoaderError4		14
#define	FileLoaderError5		15

I wouldn't, in any way, have a file loader class allocate it's buffer the way your code is doing it.

1) Code may not want the entire file in memory.

2) Code cannot control where the memory is allocated from.

You could, of-course, change the code to take an allocator object of some description. But I personally prefer the buffer pointer to be handed to the read operation.

For similar reasons I wouldn't try to hide the Read method away, but also wouldn't provide async and non-async operations into the same object, which confuses the API and can lead to incorrect usage. You could make the read* functions protected, give the object a protected constructor and derive 'AsyncFileReader' from it or take another route.

I personally prefer the route of obtaining a stream object and the user creates operations using that object. Allowing them to wait on that particular operation.

For example (pseudo code):

InputStream *stream = OpenInputStream( uri );
if ( NULL != stream )
{
      StreamOp *op = stream->Read( myBuffer, dataLength );
      if ( NULL == op )
      {
          // Error
      }
      else
      {
          // Move into wait state
      }
}
else
{
     // Unable to open stream
}

I actually don't think non-async operations are necessary at all, if someone wants to stall they can just call the operation->Wait() method for development purposes though nothing should be loaded like that in the end, and even then it should be frowned on as it's much harder to patch code to be async down the line.

[edit] I can think of many, many different ways of implementing the above code, so please don't take it that I'm suggesting it should be done exactly like that.

n!

Just from a coding standard view why are you placing an enter after a function or if conditional and the { and not on an else clause.

Well, I just ported to Visual Studio 2013 Express for C++11 support, and the IDE forces that behaviour.... It also wants a space between if and (. Prob someone at MS wants ifs and elses in the same vertical....

I thouched up my code, please let me know if it is unsafe:

class FileLoader
{
public:
	FileLoader(std::string Filename, bool Asynchronous);
	~FileLoader();

	FileLoaderCodes GetStatus();

private:
	void Tick();
	void ReadAsynch();
	void ReadSynch();

	HANDLE file;
	FileLoaderCodes Status;
	DWORD Size;
	DWORD NumberOfBytesTransfered;
	DWORD LastError;
	OVERLAPPED OverlappedInfo;
	std::vector<BYTE> buffer;
};

FileLoader::FileLoader(std::string Filename, bool Asynchronous)
{
	Status = FileLoaderCodes::Unknown;
	Size = 0;
	NumberOfBytesTransfered = 0;
	LastError = 0;
	ZeroMemory(&OverlappedInfo, sizeof(OverlappedInfo));

	if (Asynchronous)
	{
		file = CreateFileA(Filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN | FILE_FLAG_OVERLAPPED, NULL);
	}
	else {
		file = CreateFileA(Filename.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN, NULL);
	}

	if (file == INVALID_HANDLE_VALUE)
	{
		LastError = GetLastError();
		Status = FileLoaderCodes::InvalidHandle;
		return;
	}

	Size = GetFileSize(file, NULL);
	if (Size == INVALID_FILE_SIZE)
	{
		LastError = GetLastError();
		Status = FileLoaderCodes::InvalidSize;
		CloseHandle(file);
		return;
	}

	buffer.resize(Size);
	if (buffer.capacity() < Size)
	{
		Status = FileLoaderCodes::AllocError;
		CloseHandle(file);
		return;
	}

	if (Asynchronous)
	{
		ReadAsynch();
	}
	else {
		ReadSynch();
	}

}

FileLoader::~FileLoader()
{
	CloseHandle(file);
}

FileLoaderCodes FileLoader::GetStatus()
{
	Tick();
	return Status;
}

void FileLoader::ReadAsynch()
{
	OverlappedInfo.hEvent = CreateEvent(NULL, true, false, NULL);

	if (OverlappedInfo.hEvent == NULL)
	{
		LastError = GetLastError();
		Status = FileLoaderCodes::CreateEventError;
		CloseHandle(file);
		return;
	}

	if (ReadFile(file, buffer.data(), Size, NULL, &OverlappedInfo))
	{
		// Operation has completed immediately.
		Status = FileLoaderCodes::Complete;
	}
	else {
		LastError = GetLastError();
		if (LastError != ERROR_IO_PENDING)
		{
			// Some other error occurred while reading the file.
			CloseHandle(file);
			Status = FileLoaderCodes::ReadError;
		}
		else {
			//Operation has been queued and will complete in the future. 
			Status = FileLoaderCodes::InTransfer;
		}

	}
}

void FileLoader::ReadSynch()
{
	if (ReadFile(file, buffer.data(), Size, &NumberOfBytesTransfered, NULL))
	{
		// Operation has completed.
		CloseHandle(file);
		if (Size == NumberOfBytesTransfered){		//Might be 0 if EOF???
			Status = FileLoaderCodes::Complete;
		}
		else {
			Status = FileLoaderCodes::ReadSizeError;
		}
	}
	else {
		// Some other error occurred while reading the file.
		LastError = GetLastError();
		CloseHandle(file);
		Status = FileLoaderCodes::ReadError;
	}
}

void FileLoader::Tick()
{
	if (Status == FileLoaderCodes::InTransfer)
	{
		if (HasOverlappedIoCompleted(&OverlappedInfo))
		{
			if (GetOverlappedResult(file, &OverlappedInfo, &NumberOfBytesTransfered, false) != 0)
			{
				// Operation has completed.
				CloseHandle(file);
				if (Size == NumberOfBytesTransfered){
					Status = FileLoaderCodes::Complete;
				}
				else {
					Status = FileLoaderCodes::ReadSizeError;
				}
			}
			else{
				// Operation has failed? / HasOverlappedIoCompleted is true
				LastError = GetLastError();
				CloseHandle(file);
				Status = FileLoaderCodes::OverlappedResultError;
			}
		}
	}
}

enum class FileLoaderCodes
{
	Unknown,
	Complete,
	InTransfer,
	InvalidHandle,
	InvalidSize,
	ReadError,
	ReadSizeError,
	AllocError,
	CreateEventError,
	OverlappedResultError
};

The most glaring problem is that the class doesn't provide a mechanism for getting to the loaded bytes, so I would guess you haven't really thought about the client is going to interact with this class

Yes, this pertains to my second question in the original post. I am considering a method GetBuffer() which returns a shared_ptr to the vector. So I will change the code:

std::shared_ptr<std::vector<BYTE> > spBuffer;

Optionally I can have some move semantics when I want to process it, which will invalidate the object for further use.

This is a simple class that should do exactly what it is supposed to do. I won't need it for other platforms then Windows.

You seem to misunderstand std::vector::resize() and std::vector::capacity(). std::vector::resize() will always change the vector's length() unless the allocator throws an exception (the default allocator will). std::vector::capacity() will always be >= length(), so this test is not useful.

I thought this was an elegant way of avoiding new/delete[]. If the resize throws then capacity will tell me if it did right?

A related note is that most games have more than one file they wish to load. As such, you may need to be able to store FileLoader instances in a container, or perhaps the class should be modified to support loading multiple files.

My previous version had containers for this. But I wanted to go more OOP on this class and have every object manage their own buffers. I will spawn hundreds of these during init to saturate the bandwidth for fast loading times, all asynchronously.

Regarding the Status codes I think the client should be able to tell what's going on without having to go through methods. A simple switch(Status) should allow the client to decide what to do next.

The most glaring problem is that the class doesn't provide a mechanism for getting to the loaded bytes, so I would guess you haven't really thought about the client is going to interact with this class

Yes, this pertains to my second question in the original post. I am considering a method GetBuffer() which returns a shared_ptr to the vector.

My thoughts:
1) It's a reading a file, not writing a file, so the buffer should be read-only (i.e. const).
2) std::vector is an internal implementation detail. Do you really want that exposed as part of your permanent public interface? Maybe yes, maybe no, but it's worth considering.
3) I don't want the overhead of a shared_ptr when just trying to access the buffer. Example:

blah = file.GetBuffer()[20];
blah = file.GetBuffer()[23];

Each call to GetBuffer() would allocate a new shared_ptr, which wouldn't be nice.

Instead, GetBuffer() should just return a const-ref. My personal opinion would be a const-ref to the bytes directly, but a const-ref to the vector itself would be fine.

If someone wants to copy the buffer, they can explicitly do so. In this case, an explicit copy is a good thing.
You could also provide a function to explicitly take *ownership* of the data, perhaps returning the vector by move semantics and giving up ownership of it. If you find this is needed in your code.

I prefer using a custom buffer class ([header][source]) which handles ownership issues, and then having single standalone functions to load or save a file entirely into memory and return the buffer class entirely - which I use for files I know will be small in size, and separate classes that handles streaming files to and from buffers.

You seem to misunderstand std::vector::resize() and std::vector::capacity(). std::vector::resize() will always change the vector's length() unless the allocator throws an exception (the default allocator will). std::vector::capacity() will always be >= length(), so this test is not useful.

I thought this was an elegant way of avoiding new/delete[]. If the resize throws then capacity will tell me if it did right?

If resize() fails the default allocator throws, and you aren't catching it anywhere, so it'd be propagated up your callstack. i.e. if it throws, your code won't ever reach the if() statement. You could change that behavior with a custom allocator for std::vector.

I'm not sure I understand. What I'm proposing is that a single file loader instance might be capable of handling multiple file loading requests. This is a design decision that is orthogonal from object orientation.

In my previous design I had a class that did exactly this, I could submit a request and then later pick up completed files. In this design I want to do this sort of management outside because I think it will be simpler.

How, if you cannot store them in a container? The standard containers require objects to be copyable, and yours is not. Will you require dynamic allocation, too?

I want an AssetManager to handle the lifetime of these objects:

struct GameResources
{
	std::vector<std::shared_ptr<Mesh> > Meshes;
	std::vector<std::shared_ptr<Texture> > Textures;
	std::vector<std::shared_ptr<Animation> > AnimationTracks;
	std::vector<std::shared_ptr<Skeleton> > Skeletons;
};

class AssetManager
{
public:
AssetManager();
~AssetManager();


void Tick(/*.....*/);
std::shared_ptr<GameResources> GetResource(DWORD ID); //Calls GetFile if ID is not in Resources or in Transfer, return pointer is NULL if not ready.


private:
bool GetFile(DWORD ID); //table lookup ID -> Path, creates a FileLoader(Path, true) and places it on FilesInTransfer List.
void CreateResourceFromCompletedFiles(/*.....*/); //Called by Tick(), traverses FilesInTransfer and creates Resources


std::list<std::shared_ptr<FileLoader> > FilesInTransfer;
std::map<DWORD, std::shared_ptr<GameResources> > Resources;
};