Jump to content
  • Advertisement
Sign in to follow this  
Master Ar2ro

Turtle File Format

This topic is 4836 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 created a Virtual File System called Turtle File Format and released it under the GNU GPL license. If anyone is interested, please visit: http://ar2ro.niuniek.net for more details. This is my first open source project so feedback would be really appreciated.

Share this post


Link to post
Share on other sites
Advertisement
Yes, I know. I got used to using them and don't think it's a really bad thing to do. Ofcourse if you can suggest something better, I'm always open for suggestions...

Share this post


Link to post
Share on other sites
After Sneftel's comment, I tried taking a look and had a really hard time following all of what was going on, it seemed that sometimes you were using gotos to clean up and sometimes I couldn't tell why you had used one instead of a condition on the loop that you were in.

Everything that you did in the code did seem to be possible without gotos, so trying to refactor the code to not use any should really clean it up, which will probably make it easier to maintain and reduce the number of hidden bugs, so everybody wins [smile].

Share this post


Link to post
Share on other sites
Ok. I put it on top of my TODO list. Should I rather double the code than use gotos in future (also like at line 1012 'goto WRITE_COMPRESS;')?

Share this post


Link to post
Share on other sites
You should do neither. Let's examine the goto you mentioned to figure out how to remove it without doubling code:


if (m_bEnableNoCompress)
{
if (l_dwFSize > GetFileSize(l_hFile, NULL))
{
SetFilePointer(l_hFile, 0, NULL, FILE_BEGIN);
l_file.bFlags = 0;

l_dwFSize = GetFileSize(l_hFile, NULL);
ReadFile(l_hFile, l_pInBuf, l_dwFSize, &l_dwRead, NULL);

/* if th encryption flag is present, encrypt the file */
if (l_pOp->bType & TFF_ENCRYPTION)
{
l_file.bFlags |= TFF_ENCRYPTION;
TFFEnc_SimpleXorBuf(l_pOp->pKey, l_pOp->bKeyLen, l_pInBuf, l_dwFSize);
}

/* calculate the crc only after the encryption, so as not to give any info
* about the plain text */

l_file.dwCRC = crc32( l_file.dwCRC, l_pInBuf, l_dwRead );
WriteFile(hNewArchive, l_pInBuf, l_dwRead, &l_dwRead, NULL);

/* update current position in the new archive */
if (l_dwNewPosLow + l_dwRead < l_dwNewPosLow)
l_dwNewPosHigh++;
l_dwNewPosLow += l_dwRead;

CloseHandle(l_hFile);
l_file.dwCompressedSize = l_file.dwSize;
}
else
goto WRITE_COMPRESS;
}
else
{
WRITE_COMPRESS:
/* if th encryption flag is present, encrypt the file */
if (l_pOp->bType & TFF_ENCRYPTION)
{
l_file.bFlags |= TFF_ENCRYPTION;
TFFEnc_SimpleXorBuf(l_pOp->pKey, l_pOp->bKeyLen, l_pOutBuf, l_dwFSize);
}

/* calculate the crc only after the encryption, so as not to give any info
* about the plain text */

l_file.dwCRC = crc32( l_file.dwCRC, l_pOutBuf, l_dwFSize );
WriteFile(hNewArchive, l_pOutBuf, l_dwFSize, &l_dwRead, NULL);

/* update current position in the new archive */
if (l_dwNewPosLow + l_dwRead < l_dwNewPosLow)
l_dwNewPosHigh++;
l_dwNewPosLow += l_dwRead;
CloseHandle(l_hFile);

l_file.dwCompressedSize = l_dwFSize;
}


Whew. Now let's tighten that up to just the stuff affecting control flow.

if (condition1)
{
if (condition2)
{
// do something;
}
else
goto WRITE_COMPRESS;
}
else
{
WRITE_COMPRESS:
// do something else
}


Now we rewrite it to have the same behavior but no gotos:

if (condition1 && condition2)
{
// do something;
}
else
{
// do something else
}


...and the resultant code is cleaner, easier to read, and won't make programmers who don't like gotos get their panties in a bunch.

Share this post


Link to post
Share on other sites
Other comments: your header doesn't seem to require the <vector> header but includes it anyways. Also, in the header you expose a lot of details that the client doesn't need to know about. Consider pimpling or making the class an abstract base class with a generator function. Either way you don't need to expose the client to the windows header either. Also, consider exposing some versioning information somewhere. Part of the output file name is one place if you want to do the bare minimum to support versioning.

Your code is also schizophrenic. Pick one: C or C++, don't use C idioms in C++ code, especially declarations, if the code isn't intended to interface with C.

You have one at least one potential memory leak in RecursiveAddFiles() when you return when either pointer is null, but don't deallocate one that isn't.

Share this post


Link to post
Share on other sites
Ok. Thanks for the help, honestly I didn't think of that and it's really simple. Please tell me, if condition1 is false, will the program check the other condition as well or will it go directly to the else block?

Share this post


Link to post
Share on other sites
Quote:
Original post by Master Ar2ro
Ok. Thanks for the help, honestly I didn't think of that and it's really simple. Please tell me, if condition1 is false, will the program check the other condition as well or will it go directly to the else block?

It'll go directly to the else block. That's called "short-circuiting".

Share this post


Link to post
Share on other sites
Quote:

Please tell me, if condition1 is false, will the program check the other condition as well or will it go directly to the else block?


Nope, It's called short-circuiting.

if(a && b)

if a is false, b will not be checked since the expression is always false

if(a || b)

if a is true b will not be checked since the expression is always true

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.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!