Janitorial work

[charlieg]

Um if this has been pasted directly from the code [i.e. no typos] then this looks dangerous:

Code: Select all

while ((un=*ui)=NULL) { 

Is that not assigning *ui to un, then assigning NULL to un?

[safemode]

since it never runs, it's not dangerous. But if it had been run at any time, it would just null un and not execute the loop ever.


I'll just remove the function completely. No point in having a function there at all if it is never used.

[safemode]

src/cmd/unit_collide.h:259: error: 'class UnitHash3d<char [20], char [128], char [27]>' has no member named 'updateBloc'

are you saying that UnitHash3d is also never used?


class CollideTable
{
unsigned int blocupdate;
public:
CollideTable (StarSystem *ss):blocupdate(0),c(ss) {}
void Update () {c.updateBloc(blocupdate++);}
UnitHash3d <char[coltablesize],char[coltableacc], char [tablehuge]> c;
};

it seems to be used in this class as the Update function. Is this class not used either?

[safemode]

Basically, this is looking more and more to be a old bug. The effect of which is unknown at this time.

I mean, it's obvious the function only does some debugging, but it does write to the list that it's using ...so it may affect other things in the game in some way.

Should we remove it and thus the other stuff dependent on it, or fix it and leave it in?

[ace123]

Well there are a few ways to look at this. You know the unit iterators pretty well... there's no way updateBloc is doing anything except wasting clock cycles.

On the other hand, I wouldn't strip out code that doesn't take up any extra time (incrementing a variable and calling an empty inline function shouldn't be a problem)

I'll talk to my brother in a few days about what that code was supposed to do when he gets back from SIGGRAPH.

[safemode]

stripping out dead code is a healthy part of maintenance. If the code is useless, and it looks like it is, then it should go, not remain simply because it doesn't hurt gameplay. If it's debugging code, then it should be fixed and wrapped in a #if DEBUG preprocessor conditional.

Once we know for sure what the purpose of it was, then it should fall under one of those two situations. I really dont like the idea of leaving in dead code though.

[charlieg]

I'd remove dead code fairly ruthlessly. It's unecessary bloat, and in general is unhealthy for a project to keep it in. You have SVN so if you need to restore anything you have older versions of the source at your fingertips.

[chuck_starchaser]

This is a bit OT, not exactly janitorial, but...
How hard would it be for the engine to relax its case-sensitivity? This feature seems to be an unnecessary speedbump for modding, a recurrent "cause" of dataset bugs. I was just looking into a PU bug report, of several ships showing disembodied, and it took hours of my time, looking at units.csv, the directories, the meshes, the textures, ... and in the end it turns out that faction_ships.py called for example for "orionMk2h", but the ship was named "orionMk2H" in units.csv. Seen similar disasters with systems and borked jumps also emanating from this "feature". Or make case-sensitivity optional, if it touches some people's sensitivities
/OT

[ace123]

Changing case-sensitivity would require the file system layer to be rewritten, by using directory entry listings rather than a simple open. Likely, this wouldn't slow down file accesses much, but it doesn't hurt.

Instead of doing that, it would be better to use a consistent naming scheme... Ships are capitalized, additions onto ship names (mk2) can be separated by a hyphen and lowercased for example.

I might go ahead and put in a check if in DEBUG mode that will fstat() the file and compare the case sensitivity of the file it actually opened, then print a nasty error message if you are on Windows or certain MacOSX filesystems, and the case is wrong in one of the files.

[chuck_starchaser]

That would be nice. Just might add, it's not just case sensitivity at filesystem level, but python files naming entries in units.csv, for instance. Any strings used as references internally, like between python, csv and xml, having to agree in a case-sensitive way.

[safemode]

Why dont we just make all files lower case. Simple script can rename all the files and we can edit the files necessary to open them and that will be that. Everything lowercase.

why not?

[chuck_starchaser]

I'd be 100% for it. Same thing with folders. As for internal id strings, I'm not sure. It might just be a matter of changing a few functions that make hashes and comparisons to change them internally to lower case on the fly. That way they can continue to look pretty, but without the pain.

[chuck_starchaser]

But, back to the janitorial topic, one of the things I'm fond of complaining about VS's code is floats and doubles initialized to integer constants. When browsing the code and seeing foo( 0 ) I tend to assume there's a function overload foo( int ), but after long and fruitless search it turns out there's only foo( float ). Or boolean variables initialized to 0 or 1 instead of false/true...

Also, parameters having special values, like 0.0f or -1.0f to signify special things, like "get the value from somewhere else", makes the code inscrutable to someone who hasn't slept with it since childhood.

Names are also misleading a lot of the time...

Functions that return a value are called "queries", should follow a convention, like start with "get_" and end with the units...

Code: Select all

float get_remaining_strength_cm_durasteel_equiv() const //nothrow()

and ALWAYS be const.

And ideally, const functions should be "pure", that is, not only not change the state of the class, but not change the state of any of the objects passed to them as parameters. That is, when browsing code, just by looking at the name of a function one should be able to tell whether it changes anything at all, or innocently returns a value.

Commands by definition change something, and should return void *always* (use exceptions if necessary; never return codes) and have names that sound like commands...

Code: Select all

void reset();
void set_fuel_to_percent( float new_fuel_percent_val = 100.0f ) //throw()
{
    if( new_fuel_percent_val < 0.0f
    || new_fuel_percent_val > 100.0f ) throw( fuel_percent_out_of_range() );
    fuel_percent = new_fuel_percent_val;
}

And variables should never sound like commands, by the same token.

Boolean query functions and variables are the exception; their names should sound like a statement that can be either true or false, like

Code: Select all

bool spec_is_engaged; //or
bool spec::is_engaged() const; //nothrow()

If a float variable should sometimes be marked invalid, rather than use a special value it should be wrapped in a class, and be accompained by a boolean or enum for special cases.

Code: Select all

struct fuel_amount_litres
{
   typedef enum { valid, uninitialized, unknown, irrelevant, uses_solar_power } status_t;
   status_t status;
   float amount; //litres
   ...
   operator float(); //perhaps
};

One of the beauties of adhering to strict command/query separation is that you can infer a lot of stuff about a function just by the way it is used. If a function's return value is not used, it IMPLIES that it returns void, and is a command that changes its class' state. How do you know? Because the value a function returns would simply never NOT be used, because if a function returns a value, that is it's *only* purpose, and wouldn't be called for any other purpose. So, if you see a function whose return is used, you KNOW it is a pure const function that changes nothing. If no return is being used, then you KNOW it is a command that changes something.

When browsing VS code, one basically has no way to know if a function returning a value might have side-effects or not. And often they do, not only within their classes, but affecting things passed to them as pointer or reference parameters. Such functions should be split into separate command and query. Or if a parameter passed to it needs to increment an index at each call, then the whole thing needs to be looked at and refactored, because it's not right that a function should affect any of its parameters, unless it's a clearly labelled output parameter, from which no input is taken. (And even this should be exceptional... Or, not necessarily exceptional, but functions with output parameters don't belong in classes; they should be free functions.)