Not the worst code we have

At least this one has proper line breaks and indentation.

BOOL CConfig::FromFile(const CString &filename)
{
	CStdioFile file;
	file.Open(filename, CFile::modeRead);
	CString line;
	while(file.ReadString(line))
	{
		int i = line.Find("<dbtype>");
		if(i!=-1)
		{
			int i2 = line.Find("</dbtype>");
			CString type = line.Mid(i+8, i2-i-8);
			if(type == "Sql")
			{
				dbType = 0;
			}
			else
			{
				file.Close();
				return FALSE;
			}
		}
	i = line.Find("<server>");
	if(i!=-1)
	{
		int i2 = line.Find("</server>");
		server = line.Mid(i+8, i2-i-8);
	}

	i = line.Find("<database>");
	if(i!=-1)
	{
		int i2 = line.Find("</database>");
		database = line.Mid(i+10, i2-i-10);
		div = database.Mid(3, database.GetLength()-4); //remove company name prefix and instance digit suffix
	}

	i = line.Find("<dbuser>");
	if(i!=-1)
	{
		int i2 = line.Find("</dbuser>");
		dbuser = line.Mid(i+8, i2-i-8);
	}

	i = line.Find("<dbpassword>");
	if(i!=-1)
	{
		int i2 = line.Find("</dbpassword>");
		dbpassword = line.Mid(i+12, i2-i-12);
	}
}

if(server.IsEmpty()) return FALSE;
if(database.IsEmpty()) return FALSE;
if(div.IsEmpty()) return FALSE;
if(dbuser.IsEmpty()) return FALSE;
if(dbpassword.IsEmpty()) return FALSE;

int ipath = filename.ReverseFind('\\');
path = filename.Left(ipath);

file.Close();
return TRUE;

}

BTW, four words: numeric string magic constants.

Jaime

I like how it trashes the current instance of the object and returns FALSE when it doesn't like the config file.

error

Well, I like how it leaks the file handle when it doesn't like the config file.

Bulb

@joe.edwards said:

Well, I like how it leaks the file handle when it doesn't like the config file.

No, it does not. The file handle is automatic variable and as such it's destructor is called no matter how the function is exited and the destructor does close the handle. The .Close() call is somewhat superfluous at the end in this respect.
@joe.edwards said:

Always use try { } finally { } for closing.

In fact, C++ (and this is clearly C++ using MFC) has no "finally" and the idiom used here (called RAII) is strongly preferred over try/catch in C++.

tdb

What's the deal with finding empty strings from the line? Did CS eat some of your characters?

frits

It looks like a self-taught VB programmer self-learned C++.

error

It was late and I somehow mistook it for C#.

smxlong

@Bulb said:

@joe.edwards said:
Well, I like how it leaks the file handle when it doesn't like the config file.

No, it does not. The file handle is automatic variable and as such it's destructor is called no matter how the function is exited and the destructor does close the handle. The .Close() call is somewhat superfluous at the end in this respect.
@joe.edwards said:
Always use try { } finally { } for closing.

In fact, C++ (and this is clearly C++ using MFC) has no "finally" and the idiom used here (called RAII) is strongly preferred over try/catch in C++.

You're assuming the destructor does that. I assume it does not. Why? The silly class has an Open() method. If it was RAII it would instead take the filename as a constructor parameter and it wouldn't even HAVE an Open() or a Close(). I'll bet you a bag of peanuts the thing doesn't Close() unless you call Close() explicitly.

mallard

@smxlong said:

ou're assuming the destructor does that. I assume it does not. Why? The silly class has an Open() method. If it was RAII it would instead take the filename as a constructor parameter and it wouldn't even HAVE an Open() or a Close(). I'll bet you a bag of peanuts the thing doesn't Close() unless you call Close() explicitly.

The CSdioFile class is part of MFC (documentation available here: http://msdn.microsoft.com/en-us/library/01d271fb.aspx ), it inherits from CFile. CFile's Close method documentation (here: http://msdn.microsoft.com/en-us/library/sb6ka6t4.aspx ) says: "If you have not closed the file before destroying the object, the destructor closes it for you." Where's my peanuts?

CSdioFile does have constructors that take filenames, but the author of this code has chosen not to use them. Even with RAII, Open() and Close() are useful, e.g. if you want the class to have a greater scope than the filename, or you need to close and re-open the file for some reason.

@tdb said:

What's the deal with finding empty strings from the line? Did CS eat some of your characters?

That was my own personal WTF.

It should read as searching for <dbType>, <server>, <database>, <dbUser> and <dbPassword> opening and closing tags.

You'll notice the hardcoded lengths in the line.Mid() calls.

Bulb

@smxlong said:

You're assuming the destructor does that. I assume it does not. Why? The silly class has an Open() method. If it was RAII it would instead take the filename as a constructor parameter and it wouldn't even HAVE an Open() or a Close(). I'll bet you a bag of peanuts the thing doesn't Close() unless you call Close() explicitly.

No, I don't assume. I checked the documentation before I posted.

Note, that MFC started before exceptions where standard and e.g. for WinCE the compilers defaulted to no exception handling until 2003. Since opening a file needs to be able to return error, using Open instead of doing it in constructor makes perfect sense under such conditions. Even with exceptions, throwing one is slow, so you often want return value rather than exception for often-failing function like opening a file.

jasmine2501

@frits said:

It looks like a self-taught VB programmer self-learned C++.

No, you can only learn this kind of WTF from school.

alegr

@smxlong said:

If it was RAII it would instead take the filename as a constructor parameter and it wouldn't even HAVE an Open() or a Close(). I'll bet you a bag of peanuts the thing doesn't Close() unless you call Close() explicitly.

If a constructor throws an exception, the destructor is not executed. You need to catch all exceptions inside the constructor and clean up manually.

fatbull

@smxlong said:

I'll bet you a bag of peanuts the thing doesn't Close() unless you call Close() explicitly.

\Microsoft Visual Studio 8\VC\atlmfc\src\mfc\filetxt.cpp:

CStdioFile::~CStdioFile()
{
	AFX_BEGIN_DESTRUCTOR
ASSERT_VALID(this);

if (m_pStream != NULL &amp;&amp; m_bCloseOnDelete)
	Close();

AFX_END_DESTRUCTOR

}

@alegr said:

If a constructor throws an exception, the destructor is not executed. You need to catch all exceptions inside the constructor and clean up manually.

This is not an issue with RAII. Every class instance manages only one resource. If resource allocation fails, the constructor throws an exception. No cleanup is required because nothing has been allocated. If resource allocation succeeds, the constructor always completes normally and the destructor is automatically called wherever necessary.

If you need more than one resource, you'll want to use several RAII objects. This way, the compiler ensures previously allocated resources will be freed if an exception is thrown somewhere in the middle. No need for manual cleanup.

nexekho1

So what is the worst code you have? I wrote this when I was very tired post-GGJ at 2AM. It's since been replaced but I've preserved it for posterity:

[code]RAVYNFUNC RavynRandomNormal( tsRavynVector * tsOut )

{

//shut up.

uint32_t ui32NastyHack = ( rand() & 255 ) | ( ( rand() & 255 ) << 8 ) | ( ( rand() & 255 ) << 16 );

int8_t si8Extract;

memcpy( &si8Extract, &ui32NastyHack, 1 );

si8Extract |= 1;

tsOut->glfX = si8Extract;

memcpy( &si8Extract, &( ( uint8_t * ) &ui32NastyHack )[ 1 ] , 1 );

si8Extract |= 1;

tsOut->glfY = si8Extract;

memcpy( &si8Extract, &( ( uint8_t * ) &ui32NastyHack )[ 2 ] , 1 );

si8Extract |= 1;

tsOut->glfZ = si8Extract;

tsOut->glfW = 1.0f;

RavynNormalise( tsOut, tsOut );

RETURN_GOOD 0;

}[/code]

Erm, how do I add the tabs?

derula

@nexekho said:

RavynRandomNormal

Hum. Google said this to the search term "Ravyn":
@http://www.fanfiction.net/u/75251/Ravyn said:

Ravyn is a fanfiction author that has written 34 stories for Sailor Moon, Labyrinth, Rurouni Kenshin, Naruto, and Harry Potter.

:/

nexekho1

Nothing to do with me. Just a name I like (the old English spelling of Raven) I called my game engine.

@nexekho said:

So what is the worst code you have?

I answered in another thread.

@nexekho said:

Erm, how do I add the tabs?

You can preserve your formatting if you edit your post in raw HTML and use <pre>.

thosrtanner

BOOL / TRUE / FALSEin C++?

Yechh

BPFH

@thosrtanner said:

BOOL / TRUE / FALSEin C++?

Yechh

Those are fairly common macros that predate the addition of an actual Boolean type ("bool") to the C/C++ standard. Those sorts of things tend to stick around.

thosrtanner

@BPFH said:

@thosrtanner said:
BOOL / TRUE / FALSEin C++?

Yechh

Those are fairly common macros that predate the addition of an actual Boolean type ("bool") to the C/C++ standard. Those sorts of things tend to stick around.

So do C-style casts. It doesn't mean it's a good thing.