Lets start with the function definition:
bool read_file(char *p_file_name,char *file_content[FILE_NO_OF_ROWS][FILE_NO_OF_COL],const char *delimiter, int& token_count )
You are passing C-Strings and arrays of C-String. That's not a good start. Who owns the pointers? Am I supposed to free the pointers after use create them? I can't tell based on this interface.
About the only thing I can tell for sure is that token_count is an out parameter but I am not even sure if I should reset that to zero before starting (which means the user of the code does not know if they need to set it to zero before starting, which means the only safe thing to do is set it to zero (which means if you set it to zero inside the function you are wasting instructions))?
Also passing arrays through a function is dodgy. Its not doing quite what you expect. In this case you are passing char *(*)[FILE_NO_OF_COL]
through as the parameter. Which makes validation on the other side a pain. When passing arrays don't give the function a false sense of security be exact with your type (or learn how to pass an array by reference).
Also I am betting FILE_NO_OF_ROWS
and FILE_NO_OF_COL
are macros. Don't do that macros are not confined by scope. Prefer to use static const objects. Note if you use a vector all that information is stored for you.
Prefer to pass C++ objects. `std::string is always good for passing strings and why not a vector for a container of objects.
Unlike normal; I will not complain about this (as it is confined to a very strict scope). But you should be aware that its frowned upon.
using namespace std;
Why take two lines to open a file?
ifstream file_is_obj;
file_is_obj.open(p_file_name,ios::in);
// I would have just done
std::ifstream file_is_obj(p_file_name);
Declare variables as close to the point of usage as you need them. Declaring them out here seems like a waste. Also it makes the code more complex as you have to reset things manually rather than let the compiler do it.
string buffer,token;
int token_pos;
token_count=0;
Sure you can test if it is open. But usually that is a waste of time. If it failed to open nothing else is going to work. So exit the function now and reduce them number of levels of indention of the code.
if( file_is_obj.is_open())
{
This is RARELY correct in any language.
while( !file_is_obj.eof() )
{
// STUFF
}
The pattern for reading from a file is:
while( <READ Value> )
{
// Read Succeeded continue to processes code.
// STUFF
}
This is because EOF is not set until you read past the end of the file. The last successful read will read up-to but not past the end of file. Thus you have no data left to read but the EOF flag is not set and thus you enter the loop. When you attempt to read the next item it will fail and set the EOF flag. So if you are doing it your way your code should look like this:
while( !file_is_obj.eof() ) // This is now redundant.
{
<READ VALUE>
if (file_is_obj.eof()) // You have to check for read failure
{ break; // In all languages.
}
// STUFF
}
Thus it is better to use the standard pattern were you do the read as part of the test.
If you declare buffer here. Then you would not need to manually clear it here (or reset token).
buffer.clear();
token_pos=0;
Looking at your string splitting code:
for( int iter=0; token_pos != -1 || iter >= FILE_NO_OF_COL ; iter++)
You are making assumptions about the result of std::string::find()
. Why do you think token_pos != -1
will ever be correct. The documentation is very clear. If there is no match to the find it returns std::string::npos
. Dont make any assumptions on what that value is.
The standard way of testing for out of bounds is less than iter < FILE_NO_OF_COL
though your test is perfectly valid iter >= FILE_NO_OF_COL
it looks weird.
Prefer pre-increment to post increment. It makes no difference in this case. But in general siuation it can make a difference (because we use the same kind of loop with iterators). So to be consistent and always get the best loop characteristics use the pre-increment.
Would not need to clear token if it was local.
token.clear();
This bit of code is real over-complex.
token_pos=buffer.find(delimiter,0);
token=buffer.substr(0,token_pos);
if( !token.empty() )
{
buffer.erase(0,token_pos+strlen(delimiter));
file_content[token_count][iter] = new char[token.length()+1] ;
memset(file_content[token_count][iter],'\0',(token.length()+1)*sizeof(char));
token.copy( file_content[token_count][iter], token.length(), 0);
}
}
- You find the delimiter.
- You copy the token into another object.
Which would clear it (so the above clear is usless).
- You manually allocate memory
- You manually clear all the memory.
- You do a second copy.
Note this second copy is wrong (as you don't copy the terminating '\0' to make it a real C-String and thus you loose all information about its end.
So you are copying the string twice. You are manually allocating memory (with no definition of how the memory should be released).
Also any unset members of the array have there original values (which may be misleading). Either the caller of the function has to make sure that the array is correctly nulled out before calling (which you do not make clear in your interface) or you should be nulling out the undefined cells.
std::getline(file_is_obj,buffer,delimiter);
, to return the string up to but not including the delimiter? – tinstaafl yesterday