gbadev.org forum archive

Can you please review my C codes and tell me if it is good so far?
http://zzo38computer.cjb.net/powerxy/ForthBASIC/

Please tell me if something could be better than it already is, or if it is good just how it is.
_________________
Important: Please send messages about FWNITRO to the public forum, not privately to me.

Your code is not self documenting, so it should be commented.

-Brendan

sgeos wrote:

Your code is not self documenting, so it should be commented.

-Brendan

It is commented. Am I doing it wrong?
_________________
Important: Please send messages about FWNITRO to the public forum, not privately to me.

zzo38computer wrote:

It is commented. Am I doing it wrong?

This is a very subjective topic, but: Yes, as none of the algorithms and / or functions are documented. The only comment (which is not always documenting) I can see in your functions is "// Japanese."

Self documenting code is code, that has self explanatory "element identifiers" (e.g. variable and function names) and comments. (I guess, but I don't know any strict "definition".)

It should be made clear what an algorithm should do, how it does it, what it needs in order to work, what it will return and when it will fails.
Not all of this applies to every algorithm, but at least some parts should.

I'm no big "documenter" myself, but I'm trying to improve myself, as it sometimes happens that I come across a badly or even undocumented section of code and I don't know what's going on, until I read the function itself.

That's inherently bad the moment you start working for a company, as other people *will* have to read, understand and use your code and your company can't afford, that everyone has to figure out how things work on their own. It's part of your job as a programmer, even if your code is just from a hobby project, that only you will ever see. Do it for yourself.

Happy coding (and documenting)! :^D

EDIT: Wikipedia says: http://en.wikipedia.org/wiki/Self-documenting

Don't take this literally:

Wikipedia wrote:

An example of self-documenting software is TeX. When running on its own source code, TeX can produce a file with the complete printable documentation of itself.

X^D

NOTE: Personally, I like to use doxigen (a "documentation generator" or "extractor") with (as tepples mentiones below) javadoc style comments. :^)
_________________
July 5th 08: "Volumetric Shadow Demo" 1.6.0 (final) source released
June 5th 08: "Zombie NDS" WIP released!
It's all on my page, just click WWW below.

Last edited by silent_code on Sun Jun 08, 2008 2:57 pm; edited 3 times in total

At the very least, each function should have a comment before it that states what the function does, along with its preconditions and postconditions. Even if you're not coding in Java, the Javadoc style guidelines can help you organize things.
_________________
-- Where is he?
-- Who?
-- You know, the human.
-- I think he moved to Tilwick.

Wow, it doesn't even say which encoding is used for Japanese here. Is it Shift-JIS?
_________________
"We are merely sprites that dance at the beck and call of our button pressing overlord."

Code:

if(t==BTYPE_U8) {
c=*tos;
if(c>='a' && c<='z') *tos=c-('a'-'A');

Several character type and conversion routines are already present under <ctype.h>. This might be worth a look.

Code:

   int l= (int *) tos; tos += sizeof(int);
   while(l) {
      c=*tos;
      ...
      tos++; l--;
   }

Where's the escape clause for this endless loop? Oh wait, that's an ell, not a one. Unless you want to incur the wrath of the maintainer, do not use lowercase 'L' or uppercase 'o' for single-letter variables. They look too much like one and zero in many fixed-width fonts

Code:

l1I 0O

Code:

int l= (int *) tos; tos += sizeof(int);

(int*) just casts to a pointer, it does not dereference it. Did you mean *(int*)tos ? It might be better to hide the tos access behind accessor functions altogether.

Also, if this is intended for NDS use, casting to int* and reading from tos is a very bad idea. Word and halfword reads and writes are required to be 4-byte and 2-byte aligned, respectively. Since tos is accessed as bytes as well, problems are practically guaranteed. Either force word-alignment, or create routines that read/write multibyte items as separate bytes.

The whole if/else block can probably be better implemented as a look-up table.

Code:

//! Lut to do funny stuff to accented characters. Chars 0x80 to 0xDF
const u8 lut_deaccent[0x50]= {
0x80, 0x81, 0x82, 0x83, 0x8C, 0x8D, 0x8E, 0x9C, // __ __ __ __ a' e' i' o'
0x9D, 0x89, 0x8A, 0x8B, 0x8C, 0x8D, 0x8E, 0xE9, // u' __ __ __ __ __ __ ae
0x90, 0x91, 0x92, 0x93, 0x9E, 0xAC, 0xAD, 0xAE, // __ __ __ __ a` e` i` o`
0xBC, 0x99, 0x9A, 0x9B, 0x9C, 0x9D, 0x9E, 0x9F, // u` __ __ __ __ __ __ __
0xA0, 0xA1, 0xA2, 0xA3, 0xBD, 0xBE, 0xCC, 0xCD, // __ __ __ __ a" e" i" o"
0xCE, 0xA9, 0xAA, 0xAB, 0xAC, 0xAD, 0xAE, 0xAF, // u" __ __ __ __ __ __ __
0xB0, 0xB1, 0xB2, 0xB3, 0xEF, 0xDF, 0xCF, 0xEE, // __ __ __ __ a~ n~ o~ c,
0xDC, 0xB9, 0xBA, 0xBB, 0xBC, 0xBD, 0xBE, 0xAF, // a^ __ __ __ __ __ __ y"
0xC0, 0xC1, 0xC2, 0xC3, 0xC4, 0xC5, 0xEC, 0xDE, // __ __ __ __ __ __ o^ i^
0xDD, 0xC9, 0xCA, 0xCB, 0xCC, 0xCD, 0xCE, 0xCF, // e^ __ __ __ __ __ __ __
0xD0, 0xD1, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6, 0xD7, // __ __ __ __ __ __ __ __
0xED, 0xD9, 0xDA, 0xDB, 0xDC, 0xDD, 0xDE, 0xDF // u^ __ __ __ __ __ __ __
};

if( inrange(c, 0x80, 0xE0) )
*tos= lut_accent[x-0x80];

Thanks for telling me these things.

I make a list:

I fixed (int *) to *(int *) now. It is not intended to run on the Nintendo DS.
It uses NK encoding for Japanese. (My own encoding, described in a Excel spreadsheet in the parent directory)
I changed the 'l' variable to 'L' instead.
I added some documentation (including stack effects) of the functions.
The functions with "basicfn_" are functions that are called from Forth or BASIC, that is why they deal with the stack.
I added more comments in the datatypes.h file. These are BASIC types, not C types. (One difference is a BASIC string can contain embedded nulls but a C string cannot)
I didn't put the lookup table yet, but I will eventually.
Probably I should not make it convert uppercase/lowercase of accented letters of 437 encoded strings, because QBASIC doesn't convert. If you disagree, please tell me. (NK encoded strings will still convert accented letters)
This will eventually become a commercial project. It will still be licensed under the GNU GPL when that happens.

Do have any more questions/comments? Eventually I will write the rest of the codes.
_________________
Important: Please send messages about FWNITRO to the public forum, not privately to me.

Please, avoid single character identifiers! It really isn't much better to use L instead of l! I guess it has a purpose you named it L, not Q... maybe it stands for "length". It would be really, *really* better to name it "length" instead!

What i also spotted is, that you have some sort of global "in" and "out" "buffers"... I guess, it's the stack, right? Then write that down in the comments right before the functions! Chances are, some of those functions will get moved into other source files or even other implementations and then it'll become expensive!

Such "anonymous" globals (not clearly mentioned in the documentation / comment) decrease readablility. If you intend to make a commercial product with that implementation, you will *have* to expect, that this might get revised sooner or later and it may not be you to do it. So, why not make it easier to read, understand and maintain?

Now, I know this might be "necessary", but most of the time you can find a better solution to a given problem. I advise you try not using those globals (I don't say globals a bad or evil, though ;^) ).

And when using those globals, make a note (a more or less complete sentence) that those are used and where to find them, in case you forget (believe me, even programmers with hugh "head disk drives" "forget", it will eventually happen to you, too).

And one last thing I just spotted: It's code, not codes. ;^)
I tend to like "sources" more than "code" and "programming", over "coding", but that's a personal taste matter. Well, I find those more accurate, because with "codes", I tend to think of encryption... ;^D

You are lucky that Cearn took the time to examine your implementation ("code")! It's not to be taken for granted that people, who "know their stuff" do that. ;^)
(Btw: By that I don't want to say, that *you* don't "know your stuff". ;^D )

@ Cearn: Good work, man. It's really nice of you to do that. :^)
_________________
July 5th 08: "Volumetric Shadow Demo" 1.6.0 (final) source released
June 5th 08: "Zombie NDS" WIP released!
It's all on my page, just click WWW below.

silent_code wrote:

.....What i also spotted is, that you have some sort of global "in" and "out" "buffers"... I guess, it's the stack, right? Then write that down in the comments right before the functions! Chances are, some of those functions will get moved into other source files or even other implementations and then it'll become expensive!....Now, I know this might be "necessary", but most of the time you can find a better solution to a given problem. I advise you try not using those globals (I don't say globals a bad or evil, though ;^) ).....

( in -- out ) means it takes one parameter from the stack and provides output one value to the stack. If you know Forth programming you would understand it better. The basicfn_ are functions that are called from Forth and from BASIC (another module will interpret Forth and BASIC). That is why it uses the global stack pointers. If you know Forth you would know this also.
_________________
Important: Please send messages about FWNITRO to the public forum, not privately to me.

Yes, i agree that it might seem neccessary, and it's not a bad solution (I'm nowhere in a position to judge that!) at all. But you're writing a C program and the next programmer to do some work on that implementation might (like me) not know Forth! Thas is what I'm trying to say.

So, commenting / documenting your sources in a natural language would benefit you even more.

My rule of thums is, that if an advances programmer (someone who already knows the syntax and most language concepts) can take a look at your implementation and it's documentation and tell you, without any extra knowledge, what it is doing and how to use it, you've succeeded in documenting your code.

It doesn't have to be much, but it should be as easily (and fast) understandable as possible, making natural languages like English (not many people know the "math language" that well, but it's a fairly good candidate, too) a pretty much unbeatable favorite.

My intention is to help and I hope that it's transported with my posts. :^)

Last but not least: I wish you good luck with your project! :^D
_________________
July 5th 08: "Volumetric Shadow Demo" 1.6.0 (final) source released
June 5th 08: "Zombie NDS" WIP released!
It's all on my page, just click WWW below.

Just to show how subjective this thread is:

I don't think it's essential to have a comment before every function.

I think it's okay to have a few single-letter variable names in scope at a time. Long variable names sometimes get in the way. (But I do agree that lowercase L is very confusing.)

I think it's okay for modules to require that the reader is already familiar with certain domain-specific concepts, like the existence of a global stack pointer. As soon as the reader sees #include "SomeLibrary.h", they should expect to have to understand that library before reading further.

Again, modules should not repeat the same comment in every module about what the stack pointer is and how it works, but it should be documented in the header where it's defined. It's reasonable to expect readers to have an IDE that lets them find symbol definitions.

What I would do with the code:

Use enum instead of a series of #defines.

Adopt a naming convention to distinguish global variables from local variables, such as prefixing them with 'g_'.

Use 'switch' instead of if/else when possible.

Reduce nested braces by making separate functions. You can declare them 'inline' if efficiency is a problem. Try not to nest more than 3 levels deep.

Return from functions as early as possible. This reduces nesting even more because you don't need "else {}" around most of the function.

@ sajiimori: I am not trying to disagree with you, I just see that I need to elaborate a bit on what I posted, to clarify it abit. I've been motivated by your post. :^)

I hope that my posts don't sound "religious" in any way. I am rather open minded person when it comes to being lazy, I guess. :^)
Knowing when being lazy is good can save you hours of work, if done right. :^D

Single letter variables are ok, when used in fairly self-explanatory parts of an implementation. Like "i" as a couter etc. :^D
I was rather speaking in general and the afore-mentioned would be a special case, I my understanding. (Special case != rare) <- I am documenting my post... nerd! X^D

Commenting every piece of source is nonsense, I agree. Again, besides the simple things (they can make up the majority of the program), you *should* make some kind of notes, but nobody can force you to do so. Maybe except for the people that are in a position to fire you, if you don't adjust to a certain level of acceptable / common standards. Yes, I know this is a bit extreme. ;^D

About the stack pointer being mentioned "everywhere used": I didn't mean it should be documented *everywhere*, but *somewhere* would be nice, especially when asking other people, who don't have any special knowledge needed (nor a source for it), to check the implementation. ;^)

"Right before the functions" != "before every function", I should have writen "before a block of functions" or "at the top of the source file" or whatever. I guess that's due to the already demanding task of writing in a language (English) you don't use besides while being on the internet, that I sometimes don't write as precisely as I would like to. That's why I tend to edit my posts a lot later. ;^D
So, please excuse me for that. :^)

I agree (a lot) that some sort of convention *should* be used to consistently identify (everything, but let's just talk about ->) globals (which else were "anonymous", as I have written before, which is an imprecise term, due to the lack of a better one, I made it up) and I personally also use the "g_" prefix (or "s_" for static.) :^)

Sidenote: Wasn't that a Microsoft convention (someone at uni said that using "m_" for member variables was)?
_________________
July 5th 08: "Volumetric Shadow Demo" 1.6.0 (final) source released
June 5th 08: "Zombie NDS" WIP released!
It's all on my page, just click WWW below.

zzo38computer wrote:

[*]It uses NK encoding for Japanese. (My own encoding, described in a Excel spreadsheet in the parent directory)

This comment needs to be added. I had no clue what algorithm or encoding format you were using. If you need to link to an external specification, that can be acceptable.

zzo38computer wrote:

[*]I added some documentation (including stack effects) of the functions.

If the effects or purpose of a function are not self evident, the function should be commented. If you are using a strange or unusual language construct, that should also be commented.

zzo38computer wrote:

[*]The functions with "basicfn_" are functions that are called from Forth or BASIC, that is why they deal with the stack.

This comment should live somewhere. I never would have guessed.

zzo38computer wrote:

[*]I added more comments in the datatypes.h file. These are BASIC types, not C types.

Good plan. I never would have guessed.

If somebody who understands the programming language reads your code and thinks "I have no clue what is going on", it probably needs comments.

-Brendan

Some of the comments I was replying to were made by other people, silent_code. For instance, tepples said each function should have a comment before it. Of course, that has multiple interpretations, but one might say I'm replying to the spirit of the oft-encountered coding standard that 100% of functions should have a comment above them.

Most comment-related coding standards annoy me because they usually lead to stupid and redundant comments that only exist in the name of "consistency".

Code:

//--------------------------------------------------
// Method name: setPosition
// Purpose: Sets the position of the character.
// Arguments: The position to move the character to.
// Returns: None
//--------------------------------------------------
void Character::setPosition(Vector3 newPosition)
{
// Somebody please shoot me.
}

sajiimori wrote:

Most comment-related coding standards annoy me because they usually lead to stupid and redundant comments that only exist in the name of "consistency".

Code:

//--------------------------------------------------
// Method name: setPosition
// Purpose: Sets the position of the character.
// Arguments: The position to move the character to.
// Returns: None
//--------------------------------------------------
void Character::setPosition(Vector3 newPosition)
{
// Somebody please shoot me.
}

For what it is worth, that is also a great example of self documenting code. The problem with redundant comments, is that they can lead to confusion if the source is changed, but not the comments. Say sajiimori is working on a different project, and I inherit his code. Due to slightly different requirements, I need the old position:

Code:

//--------------------------------------------------
// Method name: setPosition
// Purpose: Sets the position of the character.
// Arguments: The position to move the character to.
// Returns: None
//--------------------------------------------------
Vector3 Character::setPosition(Vector3 newPosition)
{
Vector3 oldPosition = this->position;
this->position = newPosition;
return oldPosition;
}

Yes, you can argue that changing the comments is part of my job, but that creates extra work and in simple cases like this it should be evident what the funtion is doing. I do not think the comments could do a better job explaining the function than the code itself does.

-Brendan

sajiimori wrote:

For instance, tepples said each function should have a comment before it. Of course, that has multiple interpretations

Most of the time, when I say "should", I mean "SHOULD" per RFC 2119: "there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course." For a get/set method, I agree there often doesn't need to be a comment. But in the case of Character::setPosition(Vector3 newPosition), what point is newPosition relative to, and what ranges are valid? The comment could help address that.
_________________
-- Where is he?
-- Who?
-- You know, the human.
-- I think he moved to Tilwick.

sgeos wrote:

....This comment needs to be added. I had no clue what algorithm or encoding format you were using. If you need to link to an external specification, that can be acceptable....This comment should live somewhere. I never would have guessed...

I made a file called "other_comments.txt" to list these other comments.
_________________
Important: Please send messages about FWNITRO to the public forum, not privately to me.

@ sajiimori: I know you weren't writing in response to me alone. :^)

As I personally view it, the modified member function should either be named something like "setAndReturn()", with an optional "Old" at the end, or there should be a single line comment stating that the old position is returned, after the new is one set.

But it really remains a very subjective matter.

@ zzo38computer: Including a file with notes is good. :^)

EDIT: Removed some bull crap. ;^)
_________________
July 5th 08: "Volumetric Shadow Demo" 1.6.0 (final) source released
June 5th 08: "Zombie NDS" WIP released!
It's all on my page, just click WWW below.

Last edited by silent_code on Mon Jun 09, 2008 11:29 am; edited 1 time in total

zzo38computer wrote:

sgeos wrote:

....This comment needs to be added. I had no clue what algorithm or encoding format you were using. If you need to link to an external specification, that can be acceptable....This comment should live somewhere. I never would have guessed...

I made a file called "other_comments.txt" to list these other comments.

Your comments should be inlined in the code in an easy to find spot, or live in a readme file. If, for whatever reason, it makes sense to put them somewhere else, your readme should mention where that is.

silent_code wrote:

As I personally view it, the modified member function should either be named something like "setAndReturn()", with an optional "Old" at the end, or there should be a single line comment stating that the old position is returned, after the new is one set.

You are missing the forest for the trees. That was one specific example. It assumed that the code was already written and deployed, and for some reason the function needed to be changed in place. Would that happen with this function? Probably not. A small stand alone method like this no big deal, but when you deal with modifications buried in large source files, code and comments can get out of sync and confuse people (including yourself). One could argue "don't do that", although it is going to happen sooner or later unless the programmer is ultra-paranoid about it.

tepples wrote:

"there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course."

Good advice.

tepples wrote:

But in the case of Character::setPosition(Vector3 newPosition), what point is newPosition relative to, and what ranges are valid? The comment could help address that.

A description of the geometry your game uses should live somewhere, but it should not be in front of every function that geometry applies to. If this is a general character class, it may well work in different games with different geometries for some different subset of "all values of vector3", depending on the game.

-Brendan

gbadev.org forum archive

OffTopic > Can you please review my C codes and tell me if it is good?

#158244 - zzo38computer - Sun Jun 08, 2008 4:16 am

#158256 - sgeos - Sun Jun 08, 2008 12:13 pm

#158257 - zzo38computer - Sun Jun 08, 2008 12:21 pm

#158258 - silent_code - Sun Jun 08, 2008 12:46 pm

#158259 - tepples - Sun Jun 08, 2008 12:55 pm

#158264 - Dwedit - Sun Jun 08, 2008 1:35 pm

#158267 - Cearn - Sun Jun 08, 2008 4:52 pm

#158272 - zzo38computer - Sun Jun 08, 2008 5:57 pm

#158275 - silent_code - Sun Jun 08, 2008 6:32 pm

#158282 - zzo38computer - Sun Jun 08, 2008 8:01 pm

#158287 - silent_code - Sun Jun 08, 2008 8:22 pm

#158291 - sajiimori - Sun Jun 08, 2008 9:08 pm

#158294 - silent_code - Sun Jun 08, 2008 9:58 pm

#158314 - sgeos - Mon Jun 09, 2008 3:01 am

#158321 - sajiimori - Mon Jun 09, 2008 4:54 am

#158323 - sgeos - Mon Jun 09, 2008 5:34 am

#158324 - tepples - Mon Jun 09, 2008 5:46 am

#158326 - zzo38computer - Mon Jun 09, 2008 6:12 am

#158329 - silent_code - Mon Jun 09, 2008 8:58 am

#158331 - sgeos - Mon Jun 09, 2008 10:17 am