Code Review - FileHandler class

[megatron 0]

Hey guys,

I've been working on version 2 of my FileHandler class. I haven't implemented the read and write yet ( I'm open for suggestions/ideas )

Looking for criticism about style, readability, the system, you know how it goes. I'm pretty tough skinned so if you think it's tripe tell me! Just back up your claim with suggestions.

I won't be home after I post this, so I won't be able to make changed but I can read your comments through email and will reply back. Cheers.

#ifndef FILE_HANDLER_H
#define FILE_HANDLER_H

#include <fstream>
#include <iostream>

enum MODES { IN, OUT };

class FileHandler
{
private:
	unsigned int MaxHandles;
	unsigned int CurrentHandles;
	std::fstream *Handles;
	std::fstream& GetHandle(unsigned int id) { return Handles[id]; }
public:
	FileHandler();
	FileHandler(unsigned int handles);	
	~FileHandler();
	
	void OpenHandle(const char* file, int mode);
	void CloseHandle(unsigned int id);
	
	//void ReadHandle(unsigned int id); 	// Read to string? Char buffer?
	//void WriteHandle(unsigned int id);	// Perhaps both?
};

FileHandler::FileHandler()
: MaxHandles(0), CurrentHandles(0), Handles(nullptr) {}

FileHandler::FileHandler(unsigned int handles)
: MaxHandles(handles), CurrentHandles(0), Handles(new std::fstream[handles]) {}

FileHandler::~FileHandler() { delete[] Handles; }

void FileHandler::OpenHandle(const char* file, int mode)
{
	if(CurrentHandles < MaxHandles) // Check if any handles available
	{
		for(unsigned int i = 0; i < MaxHandles; i++)
		{
			if(GetHandle(i).is_open()) continue; // Find next free handle
			else
			{
				switch(mode) // Set file mode
				{
				case IN: 	Handles[i].open(file, std::ios::in); break;
				case OUT: 	Handles[i].open(file, std::ios::out); break;
				default: 	std::cout << "Error: No such mode\n"; return;
				}
				
				if(Handles[i].is_open()) // Check if successful
				{
					std::cout << "Success: " << file << " opened\n";
					CurrentHandles++;
				}
				else std::cout << "Error: " << file << " could not be opened\n";
				return;
			}
		}
	}
	else
	{
		// POSSIBLE FEATURE - Expand amount of handles
		std::cout << "Error: All handles occupied\n";
	}
}

void FileHandler::CloseHandle(unsigned int id)
{
	if(id < MaxHandles) // Check if out of bounds
	{
		if(Handles[id].is_open()) // Check if handle is valid
		{
			Handles[id].close();
			std::cout << "Handle " << id << " closed\n";
			CurrentHandles--;
			return;
		}
		else
		{
			std::cout << "Invalid closure of handle " << id << "\n";
			return;
		}
	}
	std::cout << "Error: Unknown handle ID " << id << "\n";
}

//void FileHandler::ReadHandle(unsigned int id) {}
//void FileHandler::WriteHandle(unsigned int id) {}

#endif 

[MiiNiPaa]

1) Tell us what this class is, why was it created and what problem does it solves.
2) Default constructor is useless: it creates unusable object. If amount of handles is not dynamically changes, remove default constructor altogether. It is way easier to add something to the interface than to remove something.
3) After opening new handle there is no way to tell its id. Make OpenHandle return id.
4) Why do you use manual memory management and not existing automated features (even unique_ptr would help you)
5?) Creating and allocating all handles at once might be counter productive. Allocate uninitialized storage and use in-place construction like vector does. Or just use vector to store streams: automatic memory management and expandable storage.
6) You are violating rule of 5. Missing move and copy constructors and assigment
7) No one want class to litter their console output. It makes your class virtually unusable in any application. If you want you may take additional parameter: reference to output stream which should receive output.

[megatron 0]

1)

I have created this class to keep track of files within a program. I figured something like a database program or a game would have to keep track of numerous files. I wanted to create a class that could hold and manipulate files from the same place.

2)

I understand the default constructor does nothing as of yet. I can take it out until dynamic memory management is in place for the creation of new handles.

3)

Yes, it's something I am fully aware of. I plan on changing all voids to ints which will return relevant data/error codes.

4)

Again, I am aware of the manual memory management. I cooked this up overly tired within an hour. I plan on changing the memory handling, I might use std::vector or implement my own.

5)

Is something I am going to look in to.

6)

Will be implemented, though I was unsure as the point of the class was to keep things in one place but hey, if people need it. ( Btw this wasn't made for people to use, just for myself. )

7)

Again well aware, was purely for debugging purposes. I have explained the planned behavior previous.



Thanks for the input.

[MiiNiPaa]

I was unsure as the point of the class was to keep things in one place

If you do not want mpre than one istance to be created, do not let more than one instance to be created. Make it a singleton.

Right now your class should disallow copying, but allow move.


Maybe more open modes? Like ate, app... Or instead of int make it receive (and forward to open() function) open mode, so you can write open mode directly: There is already bunch of predefined enumerations, no need to create another which does the same.

[megatron 0]

More than one class can be instatiated, I am making this for a game I am going write, I imagined usage such as:

FileHandler MapInfo;
FileHandler WepInfo;
FileHandler ...;

I didn't think I would need to copy data that is irrelevant ( Putting item files into the map files ).

I plan on doing all the open modes, I thought that was a must! I'm wondering if I can replicate the bitwise or's.

I imagine sometime like:

Open = 0b0001
Ate = 0b0010

Open | Ate = 0b0011

Just using the bits as flags.

I'm curious to what you mean with "instead of ints...". My iPhone is working against me today it won't let me copy.

[MiiNiPaa]

You can stop reinventing the wheel and use standard std::ios_base::openmode. Instead of having to learn your own enumerations, users could just use well known ones:

OpenHandle(/*...*/, std::ios_base::openmode mode)
{
/*...*/
//Drop switch statement:
    Handles[i].open(file, mode); 

///...
//Usage:
SomeInstance.OpenHandle("MyFile", std::ios::in);

More overloads would be fine too (say, accept std::string in addition to const char*)

[megatron 0]

That's genius. Exactly the kind of advice I'm looking for.

I was going to do the std::string overload. My real concern is the unimplemented Read and Write, I think I'm going to have to make those template functions so I can read into and write from different sources. I don't have much experience with templates but I felt make several of the same functions with differed overloads to perform the same operations overkill. Any ideas?

My ideal usage would be something like:

MyFileHandler.ReadHandle(handleID, char*); // Read handleID into char*
MyFileHandler.WriteHandle(handleID, std::string); // Weire string to handleID

Not sure if possible but a feature I would like:

MyFileHandler.ReadHandle(handleID, std::cout); // read file straight into cout