I'm looking through this code

[jake13jake]

const Animation* playLoopingAnimation(const char* name_cstr, int xPixel, int yPixel, bool centered, int layer)
{
string name = name_cstr;
toLower(&name);

FullAnimationData* animationData = getAnimationSettings(name.c_str());

return createNewAnimation(animationData->imageIndex, animationData->animationTime, animationData->animationTimeOffset, 
												layer, xPixel, yPixel, true, centered, false);
}

 

Okay, I'm a bit unclear on the cleanliness of all the code I'm looking through, especially on a lower level.

 

First, there's a string declared from a C string that's passed as an argument, the sole purpose of which is to convert the C string to lowercase. It's passed to the next function as a C string as well. Therefore, I'm not really getting the utility of converting it to a string at all. I mean, all the toLower function does is run the tolower function on each character of the string, which could be as easily done using a C string. Aside that a function call on each character creates an unnecessary amount of overhead (it could be done in the toLower function without a separate function call for each character).

 

In addition, converting something to string without needing to frequently operate on that string using string class functions really creates even more of an unnecessary amount of overhead. A string will pre-calculate certains things to assist in certain operations. It will try to do so efficiently. However, if you don't need to use a lot of string class functionality and often for a specific case, it is probably better just to leave it as a C string.

 

Sorry if I come off as anal to you, but at least now I actually feel confident I can contribute. I'd still need to get a feel for all the code. I also would need to be able to get it to compile, and preferably to be able to get an IDE that would be able to recognize where all the header files are (yea, I really do like Visual Studio, but I haven't found as good of a replacement for C++ apps in Linux).

 

This kind of string handling also makes me a bit nervous about how other code might be handled. I'll probably spend a lot of time simplifying and trimming everything down once I feel at one with the code, but I'd also be concerned about communicating with everyone else working on this since I am liable to making drastic changes in order to garner efficiency.

[Bak]

good points.

 

 

since the parameter is a const char*, it can't be modified as is, so a copy of the parameter will have to be made at some point, whether as a c-string or as a c++ string. Also, you mention trimming down the code, but doing what you suggested would make the code much longer and maybe less clear:

 

string name = name_cstr;
toLower(&name);

 

versus your suggestion, without tolower:

 

char name[128];
snprintf(name,sizeof(name),"%s",name_cstr);

int len = strlen(name);

for (int x = 0; x < len; ++x)
{
  if (name[x] >= 'A' && name[x] <= 'Z')
  name[x] += ('a' - 'A');
}

 

another important point, in any project, but especially in large projects like Discretion, is to optimize where it matters. If you look at the physics module, which has code executed potentially hundreds of times a second, you'll notice I spent a great deal of time optimizing at the expense of code clarity. This function, playLoopingAnimation, is called at the start of every animation that runs in a loop, so at most five times a second. Gaining 10 nanoseconds here will make little difference in overall performance. One can always optimize code more (even going down to !@#$%^&*embly sometimes), but it's a balance that needs to be achieved based on your program's demands.

 

btw I've been doing some more work on the net and filetransfer modules... expect STP to be working soon then I'll see about getting the linux version to compile for you.

[jake13jake]

good points.

 

 

since the parameter is a const char*, it can't be modified as is, so a copy of the parameter will have to be made at some point, whether as a c-string or as a c++ string. Also, you mention trimming down the code, but doing what you suggested would make the code much longer and maybe less clear:

 

string name = name_cstr;
toLower(&name);

 

versus your suggestion, without tolower:

 

char name[128];
snprintf(name,sizeof(name),"%s",name_cstr);

int len = strlen(name);

for (int x = 0; x < len; ++x)
{
  if (name[x] >= 'A' && name[x] <= 'Z')
  name[x] += ('a' - 'A');
}

 

another important point, in any project, but especially in large projects like Discretion, is to optimize where it matters. If you look at the physics module, which has code executed potentially hundreds of times a second, you'll notice I spent a great deal of time optimizing at the expense of code clarity. This function, playLoopingAnimation, is called at the start of every animation that runs in a loop, so at most five times a second. Gaining 10 nanoseconds here will make little difference in overall performance. One can always optimize code more (even going down to !@#$%^&*embly sometimes), but it's a balance that needs to be achieved based on your program's demands.

 

btw I've been doing some more work on the net and filetransfer modules... expect STP to be working soon then I'll see about getting the linux version to compile for you.

 

A constant pointer... the question is, is it pointing to a constant value?

I guess most versions of C++ will do that implicitly.

 

Concerning the physics module, I've seen scarier. The only movement system I've ever written was 8 directions for a player with a rectangular area. There's actually no need to have anything else in movement in a square tiling system, because the tangent lines of any collision are the same as the lines of a rectangle. The syntax was moveObject(dx,dy,object), where the object had fields for x, y, width, and height. It returned boolean to whether or not the player hit a wall when moving. The tileset array was directly accessible via the scripting language. Each tile had a value, there was also a built-in function to get the type of the tile (not the value)... probably implemented with a hash table (useful). I also made it so that arbitrary types of tiles could be set to be detected as a wall, and some other guy later added in specific exception !@#$%^&*ignments (!@#$%^&* closed-source games where they only give you scripting interfaces). There was also an offset value for the x and y checking. I could have improved it for higher speeds by using divide and conquer, but it didn't really matter consider no one would ever be going that fast. The damage system, that I was forced to script serverside, not a lot of people were happy with the feel of it. The really hard to grasp parts of it were how I moved dx and dy independently (but effectively), and the functions I made to check for a wall in the dx/dy areas.

 

Pretty much checked the area that the player was moving through for wall tiles, and rounded off the player's x or y values at the wall as a side effect. It's actually a lot more easy to be efficient if you don't have to check pixel specific areas for walls. Subspace, on the other hand, would need to change the x and y vectors as a side effect, and currently doesn't need to check for pixel specific areas.

 

Edit: Actually, that was for any direction, just you only had up-down-left-right to press by default. I actually made a few scripts that manipulated the motion independently using the same functions.

Edited June 5, 2007 by jake13jake

[Bak]

A constant pointer... the question is, is it pointing to a constant value?

I guess most versions of C++ will do that implicitly.

 

void clear(const char* array)
{
array[0] = '\';
}

 

will not compile. (or shouldn't, anyway)

[»Ceiu]

In this particular case, is the extra overhead that goes into creating the string object worth any time you might save by using the lcase method (which has probably been optimized like no other)? Would it be more efficient to simply copy the array and do the lowercasing at the same time? Is the string used elsewhere in the function or is it promptly thrown out?

 

I can see what Jake is saying, but as Bak said, is the extra work going to be worth the savings? Five times/sec is kind of a lot and that time will add up after a while, but it certainly won't be a bottleneck.

 

Meh, just my two cents.

[jake13jake]

A constant pointer... the question is, is it pointing to a constant value?

I guess most versions of C++ will do that implicitly.

 

void clear(const char* array)
{
array[0] = '\';
}

 

will not compile. (or shouldn't, anyway)

It depends on the compiler, but in either case, it generally causes a runtime error for trying to write to read-only memory.

Still, if a string copy is created, it will probably create a pointer for appending and iterators, a value for length, a value for capacity, a hash value for comparison with other strings, etc.

 

A C string, on the other hand, is just a pointer to a sequence of characters terminated by null and doesn't require all of the additional info. I don't think it's such a big deal, but it would definitely be a lot cleaner to stick with C strings unless there would be a lot of different string operations, etc. If there's a reason to use the string that's not toLower, it should be calculated when it's initialized, not in the middle of a stack of function calls. Although, for animation settings, what's the purpose of case insensitivity? It's just cleaner and easier to read through in the long run if there's more consistency.

 

In Java, you can't put primitive data types in maps, etc. (which irks me), but you can in C++. This advantage is huge and should be utilized.

 

For instance, the map instance singleAnimationSettings is . The aforementioned string instance name that is converted to lower as a string and then passed again as a C String to getAnimationSettings. In getAnimationSettings, it is then again looked up in singleAnimationSettings. The character array is then implicitly converted to a string. In the end I'm not sure how many times it's been a string and how many times its been a character array, just that it's been converted back and forth enough for following the code to be somewhat antagonizing.

 

@Cerium: I'm more concerned about the maintainability of the code.

Edited June 6, 2007 by jake13jake

[Bak]

It depends on the compiler

a correct compiler won't allow it

 

 

It's 5 times a second at most... 90% of seconds it will never get executed.

 

doing .c_str() doesn't take any more memory, so i'm not really converting it back to an array, just getting a pointer to the string's char array. That's sort of why a compiler should never allow changing values of a const char*, since if one could modify the array returned by .c_str(), the length/capacity/hash of the string would be incorrect.

[jake13jake]

It depends on the compiler

a correct compiler won't allow it

 

 

It's 5 times a second at most... 90% of seconds it will never get executed.

 

doing .c_str() doesn't take any more memory, so i'm not really converting it back to an array, just getting a pointer to the string's char array. That's sort of why a compiler should never allow changing values of a const char*, since if one could modify the array returned by .c_str(), the length/capacity/hash of the string would be incorrect.

Extra memory is used and calculations are performed on the declaration of a string, not so much by using .c_str(). And yea, a compiler shouldn't allow that because the compiler should be able to see that the operation doesn't work.

 

The const char * traced from one function call (it may not even be the first), up to a certain point that I haven't looked beyond yet, faces 4 conversions/extractions. It doesn't help the readability of the code. I would like to help improve this.