Jump to content
  • Advertisement
Sign in to follow this  

bad feeling

This topic is 4436 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I have a bad feeling about this code - it works, but I question its reliability and safety. Would someone kindly check it for me?
// XPad_File.cpp

#include "XPad_File.h"

XMLFile::XMLFile()
{
	this->lpData = 0;
	this->lpExtension = 0;
	this->lpFilename = 0;
}

XMLFile::~XMLFile()
{
}

void XMLFile::SetFileName(const char* name, const char* ext)
{
	if(lpFilename)
		delete lpFilename;

	lpFilename = new char[strlen(name) + 1];
	strcpy(lpFilename, name);

	if(lpExtension)
		delete lpExtension;

	lpExtension = new char[strlen(ext) + 1];
	strcpy(lpExtension, ext);
}

bool XMLFile::New()
{
	if(lpFilename)
		delete lpFilename;

	if(lpExtension)
		delete lpExtension;

	if(lpData)
		lpData = 0;

	return true;
}

bool XMLFile::Load()
{
	char* fn = new char[strlen(lpFilename) + strlen(lpExtension) + 1];

	strcpy(fn, lpFilename);
	strcat(fn, lpExtension);

	FILE* lpFile = fopen(fn, "r");
	long lSize;

	fseek(lpFile, 0, SEEK_END);
	lSize = ftell(lpFile);
	rewind(lpFile);

	char* buffer = new char[lSize];

	if(buffer == NULL)
		return false;

	fread(buffer, sizeof(char), lSize, lpFile);
	buffer[lSize] = '\0';

	lpData = buffer;

	fclose(lpFile);

	return true;
}

bool XMLFile::Save()
{
	char* fn = new char[strlen(lpFilename) + strlen(lpExtension) + 1];

	strcpy(fn, lpFilename);
	strcat(fn, lpExtension);

	FILE* lpFile = fopen(fn, "w");

	if(!lpFile)
		return false;

	if(fwrite(this->lpData, sizeof(char), strlen(lpData), lpFile) < strlen(lpData))
		return false;

	fclose(lpFile);

	return true;
}

// XPad_File.h

#ifndef __XPAD_FILE_H
#define __XPAD_FILE_H

#define XML		".xml"
#define XSLT	".xslt"

#include <memory.h>
#include <stdio.h>

#include <memory>
#include <string>
using namespace std;

class XMLFile {
	char*	lpExtension;
	char*	lpFilename;
	char*	lpData;
public:
	XMLFile();
	~XMLFile();

	bool New();
	bool Load();
	bool Save();

	void SetFileName(const char* name, const char* ext);
	char* GetFileName() { return lpFilename; };
	char* GetFileExt() { return lpExtension; };

	void SetDataPtr(char* data) { strcpy(lpData, data); };
	char* GetDataPtr() { return lpData; };
};

#endif

Thanks a bunch for any help!

Share this post


Link to post
Share on other sites
Advertisement
Right off the bat, I can see that you have a memory leak in Save() and Load(). In both of them, you're not cleaning up the FILE struct (closing it) when you return false (after the struct has been initialized). This could also cause some file access privilege errors or somethin' like that, I believe.

Share this post


Link to post
Share on other sites
If this is C++, why are you using c-style string and file functions? C++ style file streams would solve the bug Omega147 has already pointed out (std::stream's auto-close and release resources when they go out of scope.) std::string's would solve the other memory leak from never freeing the memory for your c-style strings.

As a more general note I strongly subscribe to the idea of Resource Acquisition Is Initialization. With your current code you would potentially have to write:


XMLFile * xml = new XMLFile();
xml->SetFileName("file", ".xml");
xml->Load();
// some processing
xml->New();
xml->SetFileName("file2", ".xml");
xml->Load();



Wouldn't something like this be nicer:


XMLFile * xml = new XMLFile("file", ".xml");
// some processing
xml->Load("file2", ".xml");



Having a function called New() is just asking for trouble. Using keywords with only capitalization to differentiate them makes reading code more difficult (notice how the gamedev color-coder screwed up.)

And finally (just noticed), another memory leak:


if(lpData)
lpData = 0;



Alan

Share this post


Link to post
Share on other sites
Thanx Omega147! I didn't even spot that one!

As for the lpData = 0, I don't know... the way the class is supposed to work, its not safe for me to pop in delete lpData, since that might accidentally delete user-assigned data (see SetDataPtr).

Share this post


Link to post
Share on other sites
Quote:
Original post by geekalert
As for the lpData = 0, I don't know... the way the class is supposed to work, its not safe for me to pop in delete lpData, since that might accidentally delete user-assigned data (see SetDataPtr).



void SetDataPtr(char* data) { strcpy(lpData, data); };




I skipped over this first time through. This is totally unsafe, it copies data into memory that may or may not be allocated, and may or may not be large enough to hold it. What happens if I call:


XMLFile * xml = new XMLFile();
xml->SetFileName("file", ".xml");
xml->SetDataPtr(someBuffer);
xml->Save();



?

Alan

Share this post


Link to post
Share on other sites
I fixed the SetDataPtr code, as you suggessted, thanks!

And uh, one more question...

I added a boolean variable userData that tells the class whether or not lpData belongs to the user, or the class. I think that solves the problem?

Share this post


Link to post
Share on other sites
It depends how you fixed SetDataPtr. If you implemented it like this:


void SetDataPtr(char* data) { lpData = data; };



You dont need to worry about the memory. If you implemented it like this:


void SetDataPtr(char* data)
{
if (lpData) delete lpData;
lpData = new char[strlen(data)+1];
strcpy(lpData, data);
};



You still need to free the extra memory you allocated.

As a side note, when you want to create a new memory buffer and copy a string you have been passed, look into the standard library function strdup.

Alan

Share this post


Link to post
Share on other sites
Or you could, you know, take the hint about using proper tools for the job (like std::string and std::vector). I will spoon-feed this because there are a lot of things that can best be learned about proper class design from seeing a complete example:

XPad_File.h

#ifndef __XPAD_FILE_H
#define __XPAD_FILE_H

#include <string>

// Jeez, you were already including <string> in the first place, and completely
// not using it. (strcpy, etc. live in <cstdlib>; you were probably getting them
// indirectly from "<stdio.h>", which is properly spelled <cstdio> now.
// Also, you shouldn't put "using" statements in a header file: it prevents
// anyone who includes your header (even indirectly) from *not* using whatever
// you declared.
// Oh, and you certainly don't need <memory>; that's for when you want to do
// things like overload the 'operator new'.
// My rule is to include in the header only the things that are needed by the
// header (i.e. for typenames that are referred to in there), and include
// anything else in the implementation file.

// Global *constants* can go in a header file, because they are constant...
const char* XML = ".xml";
const char* XSLT = ".xslt";
// Yeah, I used char* there. For constants it's good enough. For the bits that
// get manipulated, you want to make use of std::string.

class XMLFile {
std::string filename, extension;
// please stop using that Hungarian notation. See here:
// http://www.cuj.com/documents/s=7989/cujcexp1911hyslop/
mutable std::string data;
// "mutable" allows const member functions to modify it. This is kosher
// because it's just a caching mechanism and the "data" isn't really relevant
// to the 'logical state' of our object. See:
// http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.13

public:
// Note the interface that is provided here. It is deliberately simpler and
// makes proper use of RAII.
XMLFile(const std::string& filename, const std::string& extension)
: filename(filename), extension(extension) {}
// That's an initializer list. They are your friends.
// Note that we're already done writing the constructor as a result.

// Because std::strings can clean themselves up, you don't *need* to write,
// or even declare, a destructor, copy constructor or assignment operator.

// It looks to me like the idea is to be able to read the data from the file,
// let someone else modify it, and then pass it back for saving.
// This is probably not a very good concept for a class - especially not one
// named so specifically: there is exactly zero functionality provided that
// cares whether the file contains XML or not. Normally, plain accessors and
// mutators are a bad idea, and in any case the original interface was
// complicated by the need to think about both the file (load/save) and the
// in-memory data (get/set dataptr).

// What I'm going to do is just make a generic "read" and "write".
// The "write" will always set data and write to file. The "read" will
// return the cached file data: the cache is always valid except the very
// first time (if there was no previous write), in which case we load from
// file first. I'm also going to return the underlying string by reference,
// so as to avoid unnecessary copying.

const std::string& read() const;
void write(const std::string& s);

// It might be a better design to only update the data member on a write, and
// write back to file in the object's destructor. However, you then have to
// decide what to do with copy constructors and assignment operators.
};
#endif


XPad_File.cpp

#include "XPad_File.h"
#include <string>
#include <algorithm> // for std::copy
#include <iterator> // for istream_iterator and back_inserter
#include <iostream> // general I/O stuff
#include <fstream> // file I/O stuff
using namespace std;

string& XMLFile::read() {
if (data == "") {
// We assume the real data is non-empty, and therefore this signals that
// it hasn't been loaded yet.
// Now, behold the power of the C++ standard library:

// "Concatenate the filename and extension and extract a C-style string
// representation of the result. Using that as the file name, open a file
// object 'is'."
ifstream is((filename + extension).c_str());
// "If something goes wrong when reading the file, throw an exception.
// (EOF is ok; we want to read the whole file.)"
is.exceptions(ifstream::failbit | ifstream::badbit);
// "Copy from the file's current position until the end of the file,
// appending each character to the end of the string. (I.e., read the
// whole file into the string.)" This works because the string is
// empty to begin with; otherwise we wouldn't have started the copy.
copy((istream_iterator<char>(cin)), istream_iterator<char>(),
back_inserter(data));
}
return data;
}

void XMLFile::write(const std::string& s) {
data = s;
// We always write it to file:
ofstream os((filename + extension).c_str());
os.exceptions(ifstream::failbit | ifstream::badbit);
// EOF would be irrelevant here :)
copy(data.begin(), data.end(), ostream_iterator<char>(os));
}

// Yep, that's really everything!

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
// Global *constants* can go in a header file, because they are constant...
const char* XML = ".xml";
const char* XSLT = ".xslt";


Techinically those are not constants - someone could take XML and point it at a different string. Since it's global, that could yeild a debugging nightmare. You need:
const char * const XML = ".xml";
etc.


Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!