Wrong code should look wrong

Let the flames roll in...
Err... yeah, well I suppose you can talk about other stuff as well, maybe?

Moderator: Halleck

Post Reply
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Wrong code should look wrong

Post by klauss »

There's been too much talk about coding style and conventions lately not to post this link:

Making Wrong Code Look Wrong

Edit: I'm not saying I agree with everything there... just that it's interesting.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
charlieg
Elite Mercenary
Elite Mercenary
Posts: 1329
Joined: Thu Mar 27, 2003 11:51 pm
Location: Manchester, UK
Contact:

Re: Wrong code should look wrong

Post by charlieg »

That's just idiotic. Sorry.

If you are writing something you know to be a bit of a hack or dodgy, WRITE A F£$"£%% COMMENT.
Free Gamer - free software games compendium and commentary!
FreeGameDev forum - open source game development community
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Wrong code should look wrong

Post by safemode »

Should make a standard comment tag so that one can just grep the source and find all the intentional "hacks" for later use.

maybe

FOOHACK <name>

at the beginning of the comment block at the top of the offending code-segment.

something unique that we'll all use for the purpose. And the commentor's name. Not necessarily the author of the code, but at least who to try and talk to if we need to find someone who knows how the stuff works.
Ed Sweetman endorses this message.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

@ charlieg & safemode:
Did you guys read the article?
I just did, and I don't understand how your comments relate to it at all.

Apps Hungarian sounds semi-good to me, though, I'm just starting to wake up to another
problem: The distinction between type and purpose is not clear-cut... Suppose a variable
is prefixed cb, meaning a count of bytes. That doesn't tell me what's the purpose of
counting bytes; only the fact that it holds a count of bytes.
There's also a problem of maintenance: If I decide to change paradigms for strings, and
count characters (2 bytes each in the case of wchar), then I have to rename the variable,
as well as any function names returning it, and any function parameter names that
receive it.
Additionally, I'm all for using custom types rather than built-in ones.

Code: Select all

class character_count
{
    size_t count_;
public:
    explicit character_count( size_t initial_count );
    character_count const & operator++();
    const character_count   operator++(int);
    character_count const & operator--();
    const character_count   operator--(int);
    character_count const & operator+=(character_count const & other);
    character_count const & operator-=(character_count const & other);
    .............
};

class character_pointer
{
    character * p_;
public:
    character_pointer const & operator+=(character_count const &);
    .............
};
character_count can only be added to, subtracted from, compared, etceteras, to other character_counts.
Adding an int to it fails to compile, unless you wrap the int in a character_count constructor.
And character pointer can't be offset by an int, either; only by a character_count.
Code would be A LOT more robust and clear if people took the time to create custom types for different
quantifiable things, instead of just using ints and floats all over the place like in the vs engine, where
you could add a flightgroup to a faction and write the result to a file type identifier, and it would compile.
But as whether character_count objects should be prefixed cch, that's a good question. My personal
answer would be to just use a "c" prefix for counts; never mind specifying what they count; just to know
at a glance that it's a counter of something would be a great help (to me at least).
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Wrong code should look wrong

Post by safemode »

chuck_starchaser wrote:@ charlieg & safemode:
Did you guys read the article?
I just did, and I don't understand how your comments relate to it at all.

Apps Hungarian sounds semi-good to me, though, I'm just starting to wake up to another
problem: The distinction between type and purpose is not clear-cut... Suppose a variable
is prefixed cb, meaning a count of bytes. That doesn't tell me what's the purpose of
counting bytes; only the fact that it holds a count of bytes.
How does custom types tell you the purpose? it's just another name. if a prefix isn't good enough, a couple more letters are? then still, how does that warrant creating another layer of abstraction to slow things down for the sake of explicitness? just name the variable whatever you need it to be named to be happy.

the problem is, no name is going to tell you what you need to know without getting retardedly long. And no change to the backend of what it does is going to automatically be reflected in the variables already in use.

The solution you're looking for is clear and available documentation. You want the purpose of cbi, it should be written if it's not clear through context. If you can't understand if cbi when being used to count the bytes of a string is not counting the bytes of a string, then it's time to rethink if you know the language. Sometimes context is clear, sometimes it's not. Usually when you dont know what a variable does, it's because the context is bad, not because it's name is ambiguously confusing. Though, more often times than not, the most confusing aspects is when real datatypes are typedef'd for ease of use. which brings me to the next.
There's also a problem of maintenance: If I decide to change paradigms for strings, and
count characters (2 bytes each in the case of wchar), then I have to rename the variable,
as well as any function names returning it, and any function parameter names that
receive it.
Additionally, I'm all for using custom types rather than built-in ones.

Code: Select all

class character_count
{
    size_t count_;
public:
    explicit character_count( size_t initial_count );
    character_count const & operator++();
    const character_count   operator++(int);
    character_count const & operator--();
    const character_count   operator--(int);
    character_count const & operator+=(character_count const & other);
    character_count const & operator-=(character_count const & other);
    .............
};

class character_pointer
{
    character * p_;
public:
    character_pointer const & operator+=(character_count const &);
    .............
};
character_count can only be added to, subtracted from, compared, etceteras, to other character_counts.
Adding an int to it fails to compile, unless you wrap the int in a character_count constructor.
And character pointer can't be offset by an int, either; only by a character_count.
Code would be A LOT more robust and clear if people took the time to create custom types for different
quantifiable things, instead of just using ints and floats all over the place like in the vs engine, where
you could add a flightgroup to a faction and write the result to a file type identifier, and it would compile.
But as whether character_count objects should be prefixed cch, that's a good question. My personal
answer would be to just use a "c" prefix for counts; never mind specifying what they count; just to know
at a glance that it's a counter of something would be a great help (to me at least).
VS uses base datatypes for a very good reason .. they're extremely fast. VS needs to be extremely fast over being extremely idiot proof. Not that you're an idiot or that anyone coding for VS is, but that we need to create datatypes simply to make mis-using variables cause compiler errors as a way of making our job easier is going to hurt performance simply because us and our predecessors have been lazy at documenting what things do and what their intentions were.

I really _really_ hate typedefs ...I especially hate unecessary abstraction. It's great for stuff that doesn't need to be fast, but that's not the case in VS land. We need to be fast, and that mean we sacrifice certain things that end up putting more responsibility on our shoulders. Should we name a variable cbWhatever if it counts bytes, sure. If in the future it's determined that such a function or object should be more flexible,and they make it so, should they rename the variables that previously were not flexible ? Yes. Do they, obviously they haven't in VS.

The answer i'm trying to say is that we dont need to make the code idiot proof to code it the right way. Idiot proofing the code will only add layer upon layer, slow things down and add to it's memory footprint.


If i found out that chars were faster at being added than int's ...do you think I wouldn't look for any case where i could substitute a char ? do you think I should not do it because char's are characters and it would be confusing to use it in the manner of an incrementing variable ?

I would say that idiot-proofing and speed are mutually exclusive. But that you dont have to sacrifice maintainability for speed, just that you can't always protect your programmers from themselves. You do it when you can, (if you gotta make a class, might as well make the api secure and robust), but if you can do without then do without because speed matters here. It matters a whole lot more than making sure John the leet coder can't add a float to his int by abstracting his int variables.

ps,(edit) i know typdef's alone, dont add to speed issues, but they really annoy the shit out of me when i'm debuging things and going over code and trying to figure out what api some variable has and having to track it down because it's typedef'd in some header and defined in another. Typedef's need to be done in the file they are being used, not headers. I understand it's purpose as a way to make the code more readable and easier to type, but once it starts obfuscating my vision turns red and i start wishing i knew voodoo.

edit2: when i think of what you suggest, i think of QT vs GTK. Is QT more robust and easier to debug because it is harder to write code wrong that compiles and runs ? Yes. Gtk uses void pointers and such all over because it's just C and classes and all that jazz dont exist. But is QT a Fton slower than GTK and take more memory? Completely. Can you write clean maintainable GTK apps? Yes. Is it harder than QT? Sure. Now, speed may not matter to most UI's, but in our case speed is a factor...so while you may suggest choosing QT for all it's developer-side protection and organization and it bein easier to use, I would insist on GTK because in the end, you can improve the quality of the GTK code and still have it's speed, but even the best QT code is still going to be slow. it's just part of the tradeoff, some people can afford it...others cant.
Ed Sweetman endorses this message.
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Wrong code should look wrong

Post by safemode »

How not to code horrible garbage:

1. Write your code, because we all know that's what's going to be done first.

2. Now, look over the code, noting all the shortcut variable names you chose out of expediency and laziness. replace these with better names.

3. After debugging the code it's time to document it.

4. Create a comment block above the code block, outlining the purpose and the return values as well as inputs if there are any.

5. Throughout the actual code block, document anything that might be ambiguous, or questionable, or just plain interesting.

6. Note any hacks that needed to be done to get form 1 to 5. Make a note if these hacks are due to your code or a bug somewhere else.
Ed Sweetman endorses this message.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

safemode wrote:
chuck_starchaser wrote:character_count can only be added to, subtracted from, compared, etceteras, to other character_counts.
Adding an int to it fails to compile, unless you wrap the int in a character_count constructor.
And character pointer can't be offset by an int, either; only by a character_count.
Code would be A LOT more robust and clear if people took the time to create custom types for different
quantifiable things, instead of just using ints and floats all over the place like in the vs engine, where
you could add a flightgroup to a faction and write the result to a file type identifier, and it would compile.
But as whether character_count objects should be prefixed cch, that's a good question. My personal
answer would be to just use a "c" prefix for counts; never mind specifying what they count; just to know
at a glance that it's a counter of something would be a great help (to me at least).
VS uses base datatypes for a very good reason .. they're extremely fast.
And what makes you think that named datatypes are any less fast? The costs are at compile time.
VS needs to be extremely fast over being extremely idiot proof. Not that you're an idiot or that anyone coding for VS is, but that we need to create datatypes simply to make mis-using variables cause compiler errors as a way of making our job easier is going to hurt performance simply because us and our predecessors have been lazy at documenting what things do and what their intentions were.
Safemode, you just have no idea. You obviously don't read books and magazines about programming; you don't care about using the language's safeties; you don't care about proper naming; but you care about speed... Well, overall, the engine could easily be twice or three times as fast as it is now if it used less pointers and more references, if it implemented const correctness correctly, and if it used more templates and less inheritance. Easily could double the speed, just by making those changes; but you reject proper programming techniques that would make coding orders of magnitude less error-prone --because it might take a few more seconds to compile? Sorry, but I'm losing respect, fast.
I really _really_ hate typedefs ...I especially hate unecessary abstraction.
You're really showing how little you know about C++, or even C.
They are completely compile-time; they are completely unrelated to run-time performance.
Furthermore; they are NOT abstractions; typedefs are just syntactic sugar.
You have a (long to type) type like vector< pair< string, foo const & > >::iterator. Now suppose that's the only vector type and the only iterator type you're dealing with, within the scope of a class; so you can write

Code: Select all

typedef vector< pair< string, foo const & > > foo_vec_t;
typedef foo_vec_t::iterator foo_iterator_t
and then you can use the short names in the class. They are similar to macros, but without the risks and problems of macros.
It's great for stuff that doesn't need to be fast, but that's not the case in VS land. We need to be fast, and that mean we sacrifice certain things that end up putting more responsibility on our shoulders.
You're preaching to the converted about speed; I usually go against the established rules about optimization being the last step, and I optimize code up front. But there's a difference between knowing how to optimize code and just having incorrect beliefs about it. To know how to optimize code, you need to have at least a basic idea about what optimizations the compiler is capable of doing, so that you don't waste time optimizing things the compiler would optimize anyways, --such as when people replace array subscripts with pointer arithmetic because they think it's faster, which it isn't, and in fact can prevent compiler optimizations and be slower--; and so that you don't limit your use of good language features (e.g. typedefs) on the erroneous assumption that they negatively impact performance. And then you need to know what kinds of optimizations the compiler won't do, so you can do them, such as testing for common cases first, in long chain conditional blocks, or placing loops inside switch cases, rather than switch cases inside loops. And thirdly you need to know how to persuade the compiler to do certain optimizations, such as avoiding an extra call of a constructor in functions returning an object by value, by writing the return statement like a constructor --e.g. return Foo( a, b, c, d ); (which is basically an idiom to let the compiler know that you want it to apply the Return Value Optimization).
Good optimizing coders avoid the creation of temporaries and attendant calls to constructors and destructors by avoiding expressions like a = b + c for non-POD types; and write a = b; a += c; instead, for example. But people ignorant of the language, like the author of the collide library, you can tell them from a mile away: they write code like a = b + c with vectors and matrices, totally oblivious to the disaster that is in terms of performance; but then throw macros and old style casts all over the place, as if that was going to help run-time performance, which it doesn't (and often hurts it).
Should we name a variable cbWhatever if it counts bytes, sure. If in the future it's determined that such a function or object should be more flexible,and they make it so, should they rename the variables that previously were not flexible ? Yes. Do they, obviously they haven't in VS.

The answer i'm trying to say is that we dont need to make the code idiot proof to code it the right way. Idiot proofing the code will only add layer upon layer, slow things down and add to it's memory footprint.
Ditto.
Get a clue what you're talking about before you talk.
If i found out that chars were faster at being added than int's ...do you think I wouldn't look for any case where i could substitute a char ? do you think I should not do it because char's are characters and it would be confusing to use it in the manner of an incrementing variable ?
If chars were faster, I'd be using them too.
Except I'd be wrapping them in a template type, for safety (assertions about range), as well as extra speed, very probably.
But this is neither here nor there, since they aren't.
I would say that idiot-proofing and speed are mutually exclusive.
And you would be saying the most ignorant statement I've read in a long time. 90% of the time, language safeties are compile-time only. And bout 9% of the time they make the run-time faster, as they give the compiler more information, with which it can narrow down possibilities, and implement optimizations it couldn't have implemented otherwise. This is generally true of const-correctness and the use of references versus pointers; true of templates; --e.g. the stl is a lot faster than non-template containers that preceded it.
But that you dont have to sacrifice maintainability for speed, just that you can't always protect your programmers from themselves. You do it when you can, (if you gotta make a class, might as well make the api secure and robust), but if you can do without then do without because speed matters here. It matters a whole lot more than making sure John the leet coder can't add a float to his int by abstracting his int variables.
Ditto.

Code: Select all

int a, b;
a = apples;
b = bananas;
int c = a + b;
That's not faster than

Code: Select all

template < typename T >
class apples
{
    T t;
public:
    //default dtor/ctor ok
    scalar(){}
    explicit scalar( T const & t ) : t(t) {}
    .... operators ...
};
//etceteras, then
Apples a;
Bananas b;
Cherries c;
c = a + b; //Compile time error
And the latter could save you months of debugging time.
ps,(edit) i know typdef's alone, dont add to speed issues, but they really annoy the shit out of me when i'm debuging things and going over code and trying to figure out what api some variable has and having to track it down because it's typedef'd in some header and defined in another. Typedef's need to be done in the file they are being used, not headers.
Definitely. The best use of typedefs is as sugar in limited scopes. Typedefs at global scope in header files are almost as evil as macros.
I understand it's purpose as a way to make the code more readable and easier to type, but once it starts obfuscating my vision turns red and i start wishing i knew voodoo.
Every language feature can be misused.
edit2: when i think of what you suggest, i think of QT vs GTK. Is QT more robust and easier to debug because it is harder to write code wrong that compiles and runs ? Yes. Gtk uses void pointers and such all over because it's just C and classes and all that jazz dont exist. But is QT a Fton slower than GTK and take more memory? Completely. Can you write clean maintainable GTK apps? Yes. Is it harder than QT? Sure. Now, speed may not matter to most UI's, but in our case speed is a factor...so while you may suggest choosing QT for all it's developer-side protection and organization and it bein easier to use, I would insist on GTK because in the end, you can improve the quality of the GTK code and still have it's speed, but even the best QT code is still going to be slow. it's just part of the tradeoff, some people can afford it...others cant.
You can't generalize from a single comparison between two libraries. You can write very fast code in C++, faster than using C; almost as fast as Fortran; but you have to know what you're doing.

EDIT:
Alignment of class members is CRUCIAL to run-time performance.

Code: Select all

class foo
{
   char *pc;
   int    i;
   char  c;
   char  padding[7];
};
is about twice or three times as fast as,

Code: Select all

class bar
{
    char c;
    int   i;
    char *pc;
};
But we do the latter all over the vs engine.

And the "increased memory footprints" you mention are probably due to people using inheritance too much,
and worst of all, inheriting concrete classes from other concrete classes. Only the leafs should be concrete;
all other classes should be abstract (have at least one pure virtual function).

EDIT2:
If you care about performance, read this --memorize it:
Athlon 64 Optimization Guide (PDF)
We could double performance easily just by sorting class members by size and padding the leftover space,
placing proper alignment directives, and using prefetch instructions where appropriate.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Wrong code should look wrong

Post by klauss »

safemode wrote:How does custom types tell you the purpose? it's just another name. if a prefix isn't good enough, a couple more letters are? then still, how does that warrant creating another layer of abstraction to slow things down for the sake of explicitness? just name the variable whatever you need it to be named to be happy.

...

The solution you're looking for is clear and available documentation.

...

VS uses base datatypes for a very good reason .. they're extremely fast. VS needs to be extremely fast over being extremely idiot proof.
Ehm...
I don't think you get the point.

VS uses base datatypes to be fast. Fine, that's OK. But VS is like 15 years old. Current compilers can reduce the abstraction to what it actually is: another name for an int. Really... inspect the generated code.

So VS is anachronic. It uses anachronic development processes to be fast when modern processes are a lot less error prone - and still fast.

The solution you're proposing also doesn't solve the problem. Documentation doesn't help when developers fail to read or understand it. Which is where mistakes happen.

Having foolproof code is much more important than you think when your coders base is so fluid as in Vegastrike. We're all newbies here... we'll still be 90% newbies anytime in the future you want to pick, we're all prone to making mistakes. We really could use some foolproofing of code.

@chuck: I too felt that way when I read the article. Apps hungarian is something sensible at the time, but modern compilers can do the check for us, which is even better. Typedefs are one step in that direction, but not all the way. Boost's strong typedefs are cool though. And, @safemode, the compiler removes the abstraction from the assembly code. Really... you should inspect the output (with optimizations enabled of course). Abstraction is really nice when you don't have to pay for it in runtime.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

klauss wrote:Documentation doesn't help when developers fail to read or understand it.
More importantly, clear code --"self-documenting code"-- acts as self-updating documentation.
External documentation (as external documents, or even as comments in code) is not self-updating. It relies on coder
discipline to be up to date. You end up with things like,

Code: Select all

//add foo to bar:
bar -= foo++ * __BUGGY_ATOM__;
Secondly, not all coders --very few in fact-- know how to write comments. The vs engine is a prime example, where
comments are as enigmatic as the code itself, if not more so. Coders often lack writer skills and imagination:

Code: Select all

//add b to a
a += b;
is a downright useless comment.

Code: Select all

//add differential momentum to the momentum accumulator
a += b;
would be better; but still no chicken... The question that comments should address is "Why?", NOT "What?";
but it happens in most code I look at that comments describe what's happening, not the reasons why it's happening.
Most code documentation out there is just pure garbage; --wasted disk and screen space.
And automatic documentation generators simply formalize this very bad practice of focusing on the what instead
of the why.
The what should be self-evident from the code itself, if variables and functions have good names.

Code: Select all

momentumAccumulator += momentumDelta;
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Wrong code should look wrong

Post by safemode »

You knife the example of using the comment, and replace the comment with the variables named after the words from the comment, which is valid, but your reason for hating the comment (other than requiring programming discipline) is that it lacks the why. The why is lacking in your final example as well, it only explains the what by context. A comment will still be needed to explain the why, which is lacking in all your examples. Or should everything be self evident from the following unwritten code? such ideas are even more fervently used as bad programmer habits even more than sloppy comments.

only so much can be gleamed from properly naming things, you're still going to need to comment things worth commenting about. I always left the what's to the prototypes, or above the definitions of things, and the why's going into any sort of interesting parts of the implementation (which should be relatively rare).
Ed Sweetman endorses this message.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Wrong code should look wrong

Post by klauss »

@safemode: you're right, you still need comments, but when you say "add comments to your code" to a programmer, you most likely will end up having comments in the style protrayed by chuck's examples. Which is the same as not having comments.

And, furthermore, those comments only work for humans. Typedefs and especially strong typedefs work for the compiler as well, so the compiler can help you hunt for errors. Ain't that worthwhile?
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Wrong code should look wrong

Post by safemode »

My argument against typedefs is limited to not being able to identify the real type it refers to. Which is often something the user of the variable needs to know. If it's where i look first then fine, but if it's hidden in some header included at the other end of some include tree, then no, not fine.

I really dont want people to comment line after line of mundane code describing every step of A to get to B. I just want functions commented, especially in headers, and non-obvious code blocks. Perhaps function calls. The most important is the headers and any local functions/classes/etc. Knowing the purpose (why) and the what of a function and it's arguments and outputs makes dealing with the rest of it much easier, regardless of how much or little is commented in the code.
Ed Sweetman endorses this message.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

safemode wrote:You knife the example of using the comment, and replace the comment with the variables named after the words from the comment, which is valid, but your reason for hating the comment (other than requiring programming discipline) is that it lacks the why. The why is lacking in your final example as well, it only explains the what by context. A comment will still be needed to explain the why, which is lacking in all your examples. Or should everything be self evident from the following unwritten code? such ideas are even more fervently used as bad programmer habits even more than sloppy comments.
Indeed; I have no comment in the last example because it doesn't need one. Comments should be few and far between. If this is a physics module, and the function is called RK4, then there's no need for an explanation as to what OR as to why. The what is obvious if you know RK4, or a comment could have simply a url to wikipedia. And the why is obvious also: because we want physics.
So, in fact, even the why needs no comment with properly named things. Where there's a need for a why is where something in the code is AND needs to be unintuitive, or counterintuitive. Or to explain the context in which a class is used. For instance, there are classes in the vs engine that are created during an ai's maneuver, and exist for the lifetime of the maneuver. There's no way to describe such a thing through a class name; it needs to be explained in a comment block.
In the case of physics code, with proper names, there should be no need to explain why. The code should be obvious and intuitive.
Inside the code, the only comment lines I'd put are to separate the main stages in the processing, and explain momentum.
Simplified (Euler) example:

Code: Select all

RigidBody::Euler( dt )
{
    //adding force vectors 
    for( force_iterator_t i = forces.begin(); i != forces.end(); ++i )
        force_accumulator += *i;
    ..<rotational forces code>..
    //integrating... (remember that momentum's derivative IS force)
    momentum += force_accumulator * dt;
    ..<spin code>..
    //updating speed vector
    speed = momentum * inverse_mass;
    ..<rotation speed code>..
    //updating position
    position_vector += speed * dt;
    position_quaternion += rotation_speed_quat * dt;
}
That looks like too many comments; but in the case of RK4 there'd be more lines of code between the comments.
Admittedly, though, comments shouldn't be necessary here; they just help the eyes know which subsection to go
to when looking for something. The why in general is obvious. The why in the particulars can be read from a
wikipedia link.
only so much can be gleamed from properly naming things, you're still going to need to comment things worth commenting about.
True; but compare my code above to a random snippet of current engine code:

Code: Select all

    } else {
        viewStyle = CP_TARGET;
        thismode.back() <<= 1;
        while ( !(thismode.back()&posmodes) ) {
            if (thismode.back() > posmodes)
                thismode.back() = 0x1;
            else
                thismode.back() <<= 1;
        }
    }
What's ..".back()"? What's "posmodes"? Why write "1" in one place and "0x1" in another? What's being shifted and why?
Shifting to the left; then ANDing with a mask; then comparing to the same mask for numerical greater(ness)-than...
... then shifting to the left again... :evil: And a name like "posmodes"... Positive modes? Possible modes? Piece-of-shit modes?
Not to speak of the fact that back() seems to be a function returning a non-const reference to something... Evil in every way.
And that's just a tiny snippet of a function taken totally at random; from the file that was open in Gedit, middle of the page.

Documenting this would need more work than rewriting it.
Heck, even reading documentation for such garbage would be more painful than rewriting it.
Much more painful.
Code like that would need two lines of comments for every line of code, to explain why the variable is shifted,
anded and compared in such strange ways.

You see? BAD code needs comments badly, in huge numbers; and doesn't have them; --or they don't help.
Good code doesn't need comments, for the most part, but usually has some, anyways.
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: Wrong code should look wrong

Post by safemode »

yea, commenting bad code is going to be hard, but if your argument is that we rewrite everything then what the hell is the point in even talking about releasing, because we wont ever be doing one.

At some point you have to stop the rewriting, create a known state and make something releaseable before diving back into rewriting everything.

I know at every turn you see something that is begging to be rewritten ...2 years after our last release is not the time for it. We need to show some self control about what gets done in the immediate future so we can reset that clock of last release back to 0, then start indulging again.



I part of me just really doesn't care at all if we dont make another release for another 2 years. This is the part that wants to fix shit, like you do, and to not just make it work without modifying much so not much breaks. Ground up rewriting. Renaming variables / functions and such all the way down to fixing how python interfaces with us. Then the other part of me reminds the first part that we've gone from many mods and a decent amount of artists and other media contributors down to 1 of each basically for that very reason. We need the mods to increase the interest in VS because the VS game itself, is boring as hell and recieves almost no developer time (the tutorial guy is probably the most work the vs game has had in over 2 years ). Obviously the majorit y of the mods couldn't hold interest while the game sat stagnant between 0.4.3 and 0.5, and by then, couldn't make the switch with whoever remained (cept 1).
Ed Sweetman endorses this message.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Wrong code should look wrong

Post by klauss »

Wow... that's evil code you found chuck ;)

Anyway, back() is part of the STL, it's not evil, if you know the STL (which has some nasty patterns I agree - but back() is handy).

A better way to use back() helps the reading:

Code: Select all

...
posmodes.push_back(Mode(<blah>));
Mode &newmode = posmodes.back();
...
newmode >>= 1;
...
newmode <<= 1;
At least you know what back() means (it's the newly added mode). I know it isn't much. You also catch the weird bug if, for some reason, someone appended something after the first push_back.

I know, you'd say it's better to

Code: Select all

...
Mode newmode(<blah>);
...
newmode >>= 1;
...
newmode <<= 1;
...
posmodes.push_back(newmode)
Perhaps... but it's not always so.
For instance, the final copy may be expensive. back() lets you get rid of that copy and work inplace.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

@safemode: Sure; I wasn't suggesting we rewrite the engine before releasing. I was just trying to make a point about the purpose of comments and documentation --the Why-- and about self-documenting code by clearly naming things --the what--. As for the immediate course to follow, I agree we should stay the course. Documenting the code, for now, should be limited to a few lines at the top of each file, explaining in general terms what the classes do, their typical lifetimes, what other parts of the code use them, etceteras. At this junction, I think breadth is more important than depth. 3 lines of comments at the top of each file should be better than 30 lines at the top of 10% of the files; --just a little something to begin getting a general idea how everything fits together.

@klauss: I like your first solution best. We could use that trick to improve a lot of code.
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

klauss wrote:Wow... that's evil code you found chuck ;)
It's not exceptional at all; look at this function in quadsquare.cpp:

Code: Select all

/**
 * Returns the height of the heightfield at the specified x,z coordinates.
 * Can be used for collision detection
 */
float quadsquare::GetHeight( const quadcornerdata &cd, float x, float z, Vector &normal ) //const
{
    int   half = 1<<cd.Level;
    float lx   = (x-cd.xorg)/float(half);
    float lz   = (z-cd.zorg)/float(half);
    int   ix   = (int) floor( lx );
    int   iz   = (int) floor( lz );
    //Clamp.
    if (ix < 0) return -FLT_MAX;       //ix = 0;
    if (ix > 1) return -FLT_MAX;       //ix = 1;
    if (iz < 0) return -FLT_MAX;        //iz = 0;
    if (iz > 1) return -FLT_MAX;          ///iz = 1;

    int index = ix^(iz^1)+(iz<<1); //FIXME gcc computes ix^((iz^1)+(iz<<1)).. Was this the intent? Who can understand this code? --chuck_starchaser
    if (Child[index] && Child[index]->Static) {
        //Pass the query down to the child which contains it.
        quadcornerdata q;
        SetupCornerData( &q, cd, index );
        return Child[index]->GetHeight( q, x, z, normal );
    }
    //Bilinear interpolation.
    lx -= ix;
    if (lx < 0) lx = 0;
    if (lx > 1) lx = 1;
    lz -= iz;
    if (lx < 0) lz = 0; //FIXME did this mean to say "if (lz < 0) lz = 0;" ? It says "if (lx < ..." which is false --chuck_starchaser
    if (lz > 1) lz = 1;
    float s00, s01, s10, s11;
    switch (index)
    {  .................
First off, why would a function named GetHeight need so many bloody parameters? It is a member
function of a class; it should have most of the data it needs to "get the heigth" for you, except for
the x,z coords? And even so, the name should be "getHeightATxz( local_coord x, local_coord z )"...
float x, float z ... I thought the standard was x and y, or u and v... What could (x,z) possibly mean?
No explanation. No phone number to call.
//const commented out... Which tells me the author tried to make the function const, but encountered
difficulties and simply gave up.
"half" ... WTF?
lx and lz are those x and z minus some weirdness and divided by half (multiplied by two? :))
then ix and iz are the integer parts of lx and lz, which apparently can only be zero or one,
or else the function returns minus infinity.
But what gets me is that line, computing some kind of index????

Code: Select all

int index = ix^(iz^1)+(iz<<1);
And gcc is telling me...

Code: Select all

/gfx/quadsquare.cpp:220: warning: suggest parentheses around arithmetic in operand of ‘^’
Which means that, according to gcc, the entire expression after ix^ is the xor argument. Is this what the
author intended?

There's a reason, I think, comments are never about the why, the intent. I think the true intent was to
pretend to be open source, get free hosting and free help, while purposely obfuscating the code so that
nobody would be able to use it...
Success!!
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: Wrong code should look wrong

Post by klauss »

That's BSP code isn't it?

We should be getting rid of it, we should use opcode for all BSP-like queries. It has better structures. And better code.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: Wrong code should look wrong

Post by chuck_starchaser »

No idea what it is. I'd be happy to get rid of anything that can be got rid of, tho.
Post Reply