On code style

[thebigh]

 

Uff... hard-coded "magic numbers" like this always make me uneasy.

[peter_spy]

I'd say "writeFloat" method that returns void is bigger cringe here.

[STiFU]

Honestly, I wouldn't be so quick to judge here, as noone knows the history of this code-snippet. It might just have been that there were floating-point-quantization errors  in dmap and that they wanted to get rid of those by rounding at the desired precision, i.e., 0.001. Also, floating point comparisons with fixed precision like this are not uncommon at all. Some applications simply don't require more accuracy so you skip the very complex "regular" floating point comparison.

[nbohr1more]

Dhewm3:

static void WriteFloat( idFile *f, float v )
{
    if ( idMath::Fabs(v - idMath::Rint(v)) < 0.001 ) {
        f->WriteFloatString( "%i ", (int)idMath::Rint(v) );
    }
    else {
        f->WriteFloatString( "%f ", v );
    }
}

Hmm... The original GPL Doom 3 code:

static void WriteFloat( idFile *f, float v )
{
    if ( idMath::Fabs(v - idMath::Rint(v)) < 0.001 ) {
        f->WriteFloatString( "%i ", (int)idMath::Rint(v) );
    }
    else {
        f->WriteFloatString( "%f ", v );
    }
}

[nbohr1more]

ID_INLINE float idMath::Rint( float f ) {

return floorf( f + 0.5f );

}

 

TDM

ID_INLINE float idMath::Round( float f ) {
    return floorf( f + 0.5f );
}

Dhewm, GPL, and TDM all manipulate float data differently after this function. ( For example ) Nothing like this in GPL or Dhewm:

errors += (idMath::Round(xf) != xi);

I think each project copes with precision conversion differently. GPL is sadly 32-bit exclusive and uses the obsolete 80-bit float feature from Pentium 4 CPU's that was not carried forward to 64-bit MSVC compilers after 2012 as I recall. (lots of fuzzy memories about this )

 

[HMart]

11 hours ago, peter_spy said:

I'd say "writeFloat" method that returns void is bigger cringe here.

Why? Can you explain?

Personally I see nothing wrong with that, looking at that function is obvious that they made it to not return anything, it just writes a float value to a file, and in C any function that doesn't return anything returns void.

And those inner write functions may have error handling of some kind, so no need to return a bool for success or failure by the main function.

The only potencial problem I see with it is they pass the file handle by pointer and there's no guard there for a eventual null pointer being passed to the function and it will crash if that happens but the fact this worked for years tells me they made sure that never happens.

[peter_spy]

It seems to me like this is ignoring basic naming conventions in programming, stuff I was taught during my first days of work. If a method gets or sets a value of a certain type, it returns that value type. On the similar principle, all methods that return boolean always start with isSomethingSomething, etc.

[OrbWeaver]

I've very rarely seen setter methods which return values (unlike getters which obviously need to return the value they are "getting").

What value should a setter return? The same value it was given as a parameter? That's entirely pointless because the calling code already has that value. It could return the previous value, but such a value isn't necessarily defined (and doesn't appear to be relevant in the case of writing something to a file).

Sometimes setters return the object itself, so you can call them in a chain, e.g.

myObject.setWidth(60).setHeight(20).setColour(RED);

but it's not clear how that would work with writeFloat which isn't an object method to begin with.

Quote

all methods that return boolean always start with isSomethingSomething, etc.

That's certainly common (and is a convention I use), but not universal. The C++ standard library doesn't use it, for example — to check if a vector is empty you call std::vector<T>::empty(), not isEmpty().

[peter_spy]

1 hour ago, OrbWeaver said:

What value should a setter return? The same value it was given as a parameter? That's entirely pointless because the calling code already has that value. It could return the previous value, but such a value isn't necessarily defined (and doesn't appear to be relevant in the case of writing something to a file).

Sometimes setters return the object itself, so you can call them in a chain

Ah, you're right, it can be more language-specific or even project-specific. We often used setters that returned this, so we could use method chaining, but this isn't a general rule.

[HMart]

Orbweaver said all what I thought, I also hardly always follow programing "rules", mostly because "basic naming conventions in programming" change with times and as the languages evolve and change. So imo no one should get to much hung up on those, specially when some rules aren't even evidence based but some famous coder thought it made the code "cleaner" and people just followed.

For example Hungarian notation, so many coders use it but to me it can crash and burn. I played around with Penumbra HPL engine for a time and it is riddled with such notation and not once, I felt it helped me understand better the code, for the contrary.

[peter_spy]

Just now, HMart said:

I also hardly always follow programing "rules"

They are there for a reason though, not just because someone said so. If you want to communicate with people and you work on the code with someone, you need to share some common principles. And while I agree that things like clean code can be a bit extreme at times, I've never seen anyone questioning it super hard; neither stuff like solid principles, for example. Obviously, you can be a rebel if you want to, but you'll probably end up working alone.

[STiFU]

This is an intereting discussion. So, could one of the mods please move this to a separate thread as to not derail the beta testing thread.

1 hour ago, HMart said:

Hungarian notation, so many coders use it but to me it can crash and burn

As a developer, you spend about 80% of your time just reading code. So, optimizing for readability is very important. The more information you can gather just by reading without cluttering up the code (by needless comments) the better. Hungarian notation helps immensely to quickly prase the displayed code in your brain. Yes, your IDE will show the desired information as well, but a mechanical interaction with the code is required to show it (mouse over or similar). Also, often you are tasked to do code review on webpages that don't offer these nice crossreferencing features of your IDE.

 

1 hour ago, HMart said:

I also hardly always follow programing "rules"

Some things are objectively bad, 'though, so the respective rules preventing those things should universally be followed. For instance, much legacy code is nested extremely because at some point in time, it must have been a rule that you only have one return-point in your function (probably a c-residual). Such code is exhausting to read ("what was the else-if-condition some 1000 lines ago leading to this else-case again?") and very likely contains tons of code duplication. Today, we have the rule to only use little nesting and short if-else-clauses, to make the code easy to read and debug. If I come accross such a nested legacy function, I refactor the shit out of that function while trying to understand it, just so the next person after me doesn't have to deal with that horrible mess again.

[Baal]

14 minutes ago, STiFU said:

As a developer, you spend about 80% of your time just reading code. So, optimizing for readability is very important. The more information you can gather just by reading without cluttering up the code (by needless comments) the better. Hungarian notation helps immensely to quickly prase the displayed code in your brain.

That is interessting. I also do not find hungerian notation helpful in any way. Exactly the opposite in fact. 

I was doing a lot of functional programming recently, where code is very short and concise. Maybe that's the reason. It is simply not needed there.

[OrbWeaver]

I'm surprised to hear anyone is using Hungarian notation these days. It's one of the most derided, disliked, obsolete and useless naming conventions in programming. I don't think I've ever seen a modern programming guide or tutorial which advocates it (except perhaps in very limited situations like using "mSomething" to indicate a member variable).

[STiFU]

6 hours ago, OrbWeaver said:

I'm surprised to hear anyone is using Hungarian notation these days. It's one of the most derided, disliked, obsolete and useless naming conventions in programming. I don't think I've ever seen a modern programming guide or tutorial which advocates it (except perhaps in very limited situations like using "mSomething" to indicate a member variable).

That's because all those guides apparently did not reach the age of web-page-based Code-Reviews via Github, Azure Devops, and the likes...  All the people arguing against Hungarian say that you simply don't need it anymore, because your compiler / IDE will tell you what type a variable has (your Pull-Request web-gui will not show you that information) and that Hungarian only makes it harder to read variables (which is not really accurate, as our brain is perfectly capable to read any camel-case variable or function, so why shouldn't it be able to parse the prefix?).

[HMart]

7 hours ago, OrbWeaver said:

I'm surprised to hear anyone is using Hungarian notation these days. It's one of the most derided, disliked, obsolete and useless naming conventions in programming. I don't think I've ever seen a modern programming guide or tutorial which advocates it (except perhaps in very limited situations like using "mSomething" to indicate a member variable).

First I need to say sorry for keeping this of topic subject going, I promise this is the last comment from me on this. 

Now I also got surprised but Frictional Games still does, at lest in HPL1 and HPL2 engines they do, I assume they still do it on their latest engine as well, only them know why.

[stgatilov]

It turns out we have many people who enjoy discussing code

 

My complaint about that code is that all floating point numbers are snapped to integers with same tolerance.

The .proc file contains a lot of different data: models, shadow models, BSP tree, visportals.
They contain in different combinations: positions, texcoords, normals, distances.
Snapping or comparing all of these quantities with same tolerance is a really bad idea.

Some data is more sensitive to changes than other.
They even have different measurement units, i.e. you position is measures in something like meter-units, while texcoords are usually measured in something like tex-units, and they are as incomparable as meters and seconds.

Some data  should not be snapped at all because that could break its internal invariants.
If you snap coordinates of normal vector, it is no longer normalized, and the receiver has all the fun of dealing with extremely rare cases which normally should not happen.

As for Hungarian notation...

I do some mesh processing with halfedge data structures.
When you have 10 local variables being halfedge handles, 5 local variables being face handles, and 5 local variables being vertex handles, it becomes much easier to understand what's going on if they start with "he" / "f" / "v" respectively.
But this is very situational of course.

Just starting pointers with 'p' and array-like stuff with 'a' is usually unnecessary, unless you have 3-4 levels (triple pointer or triple arrays), although in that case the code is smelly regardless or how you name variables

[OrbWeaver]

Quote

My complaint about that code is that all floating point numbers are snapped to integers with same tolerance.

Indeed: there are already accuracy issues as soon as you use floats (especially single precision), and a hard-coded snap-to-integer behaviour seems to be reducing accuracy still further.

Quote

I do some mesh processing with halfedge data structures.
When you have 10 local variables being halfedge handles, 5 local variables being face handles, and 5 local variables being vertex handles, it becomes much easier to understand what's going on if they start with "he" / "f" / "v" respectively.
But this is very situational of course.

That's actually a rare example of "App Hungarian notation" where the prefixes convey meaningful application-specific information, rather than just duplicating basic types like integer or float. It has been suggested that this was what HN was originally supposed to be, but everybody used it just to show primitive types which resulted in the whole convention becoming disliked.

Quote

Just starting pointers with 'p' and array-like stuff with 'a' is usually unnecessary, unless you have 3-4 levels (triple pointer or triple arrays), although in that case the code is smelly regardless or how you name variables

Exactly, and it breaks down completely once you start using more complex types than primitive numbers. Knowing that something is a pointer is useless without knowing what it points to, knowing that something is an "obj" or a "struct" is similarly unhelpful, and once you start trying to expand the convention so that the prefixes can convey information about the actual object type, you will end up with complex multi-character encoded prefixes that nobody else will be able to understand.

[HMart]

Perhaps id didn't cared for that loss on accuracy, at lest for Doom 3, is hard for me to believe they did that for lack of knowledge or something but who knows.  Is this code still in BFG?

[STiFU]

2 hours ago, OrbWeaver said:

Exactly, and it breaks down completely once you start using more complex types than primitive numbers. Knowing that something is a pointer is useless without knowing what it points to, knowing that something is an "obj" or a "struct" is similarly unhelpful, and once you start trying to expand the convention so that the prefixes can convey information about the actual object type, you will end up with complex multi-character encoded prefixes that nobody else will be able to understand.

Of course, identifying struct via HN is useless. At work, we use HN when there are reasons for caution:

• u for unsigned to prevent unintentional implicit unsigned promotion (and overflow)
  • Yes, I know, the compiler will issue a warning, but in some legacy projects, there are tons of warnings, so devs are going to miss that one new one. We are slowly working toward warning-free builds, but as long as we are not there yet, the u-prefix helps a lot.
• p for pointers to make the reader / dev aware that that variable could be NULL.
• m_ for members, obviously useful. Similarily, g_ and s_ for globals and statics respectively, although it is obviously discouraged to use those.
• Various prefixes for the different coordinate systems we use.
• I for interface, just for convenience
• b for boolean, this is basically just for convenience so you can directly see that this is the most basic type. This one, one could defintely argue against, but I personally like it.

[nbohr1more]

1 hour ago, HMart said:

Perhaps id didn't cared for that loss on accuracy, at lest for Doom 3, is hard for me to believe they did that for lack of knowledge or something but who knows.  Is this code still in BFG?

BFG has no Dmap source code... checking RBDoom3BFG.

RBDoom3BFG uses the same as vanilla

[peter_spy]

Btw. Justin Marshall (aka icecoldduke), ex-id engine programmer, started a wiki for OG idtech4/5 engines, and he included a style guide there. Not sure whether this was used at id, but it might be useful.

http://doomarchives.com/home/doom3/styleguide

[STiFU]

If there ever was a common code-style, it is definitely lost forever in the TDM-codebase. It's a huge mess in this regard!

[nbohr1more]

Before Doom 3 was GPL, this was the guide:

https://wiki.thedarkmod.com/index.php?title=Coding_in_the_SDK

[thebigh]

Although consistency is nice, if it's a choice between clarity and consistency I choose clarity.