Wrong code should look wrong
Moderator: Halleck
-
- Elite
- Posts: 7243
- Joined: Mon Apr 18, 2005 2:40 pm
- Location: LS87, Buenos Aires, República Argentina
Wrong code should look wrong
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.
Making Wrong Code Look Wrong
Edit: I'm not saying I agree with everything there... just that it's interesting.
-
- Elite Mercenary
- Posts: 1329
- Joined: Thu Mar 27, 2003 11:51 pm
- Location: Manchester, UK
- Contact:
Re: Wrong code should look wrong
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.
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
FreeGameDev forum - open source game development community
-
- Developer
- Posts: 2150
- Joined: Mon Apr 23, 2007 1:17 am
- Location: Pennsylvania
- Contact:
Re: Wrong code should look wrong
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.
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.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
@ 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.
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).
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 &);
.............
};
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).
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
-
- Developer
- Posts: 2150
- Joined: Mon Apr 23, 2007 1:17 am
- Location: Pennsylvania
- Contact:
Re: Wrong code should look wrong
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.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.
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.
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.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.character_count can only be added to, subtracted from, compared, etceteras, to other character_counts.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 &); ............. };
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).
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.
-
- Developer
- Posts: 2150
- Joined: Mon Apr 23, 2007 1:17 am
- Location: Pennsylvania
- Contact:
Re: Wrong code should look wrong
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.
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.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
And what makes you think that named datatypes are any less fast? The costs are at compile time.safemode wrote:VS uses base datatypes for a very good reason .. they're extremely fast.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).
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.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.
You're really showing how little you know about C++, or even C.I really _really_ hate typedefs ...I especially hate unecessary abstraction.
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
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).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.
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).
Ditto.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.
Get a clue what you're talking about before you talk.
If chars were faster, I'd be using them too.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 ?
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.
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.I would say that idiot-proofing and speed are mutually exclusive.
Ditto.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.
Code: Select all
int a, b;
a = apples;
b = bananas;
int c = a + b;
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
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.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.
Every language feature can be misused.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.
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.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.
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];
};
Code: Select all
class bar
{
char c;
int i;
char *pc;
};
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.
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
-
- Elite
- Posts: 7243
- Joined: Mon Apr 18, 2005 2:40 pm
- Location: LS87, Buenos Aires, República Argentina
Re: Wrong code should look wrong
Ehm...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.
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.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
More importantly, clear code --"self-documenting code"-- acts as self-updating documentation.klauss wrote:Documentation doesn't help when developers fail to read or understand it.
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__;
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;
Code: Select all
//add differential momentum to the momentum accumulator
a += b;
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;
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
-
- Developer
- Posts: 2150
- Joined: Mon Apr 23, 2007 1:17 am
- Location: Pennsylvania
- Contact:
Re: Wrong code should look wrong
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).
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.
-
- Elite
- Posts: 7243
- Joined: Mon Apr 18, 2005 2:40 pm
- Location: LS87, Buenos Aires, República Argentina
Re: Wrong code should look wrong
@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?
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?
-
- Developer
- Posts: 2150
- Joined: Mon Apr 23, 2007 1:17 am
- Location: Pennsylvania
- Contact:
Re: Wrong code should look wrong
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.
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.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
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.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.
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;
}
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.
True; but compare my code above to a random snippet of current engine code:only so much can be gleamed from properly naming things, you're still going to need to comment things worth commenting about.
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;
}
}
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... 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.
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
-
- Developer
- Posts: 2150
- Joined: Mon Apr 23, 2007 1:17 am
- Location: Pennsylvania
- Contact:
Re: Wrong code should look wrong
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).
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.
-
- Elite
- Posts: 7243
- Joined: Mon Apr 18, 2005 2:40 pm
- Location: LS87, Buenos Aires, República Argentina
Re: Wrong code should look wrong
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:
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
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.
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;
I know, you'd say it's better to
Code: Select all
...
Mode newmode(<blah>);
...
newmode >>= 1;
...
newmode <<= 1;
...
posmodes.push_back(newmode)
For instance, the final copy may be expensive. back() lets you get rid of that copy and work inplace.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
@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.
@klauss: I like your first solution best. We could use that trick to improve a lot of code.
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
It's not exceptional at all; look at this function in quadsquare.cpp:klauss wrote:Wow... that's evil code you found chuck
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)
{ .................
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);
Code: Select all
/gfx/quadsquare.cpp:220: warning: suggest parentheses around arithmetic in operand of ‘^’
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!!
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
-
- Elite
- Posts: 7243
- Joined: Mon Apr 18, 2005 2:40 pm
- Location: LS87, Buenos Aires, República Argentina
Re: Wrong code should look wrong
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.
We should be getting rid of it, we should use opcode for all BSP-like queries. It has better structures. And better code.
-
- Elite
- Posts: 8014
- Joined: Fri Sep 05, 2003 4:03 am
- Location: Montreal
- Contact:
Re: Wrong code should look wrong
No idea what it is. I'd be happy to get rid of anything that can be got rid of, tho.
Latest version of Cinemut Opaque
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.
Latest version of LaGrande noodleworks (scroll down).
An evolving La Grande How-To...
The non-working, but latest, CineMut test_bike
PU (Privateer: Parallel Universe's Home). WC or Privateer Drayman for you?
WCpedia --The Wing Commander Encyclopedia-- From Angel Deveraux through Belisarius to Zachary Banfeld...
WC Nexus forum, the Moonbase Tycho of WC fans.