Improved smooth_economy patch

Jango · Post by **Jango** » 20 Jun 2005 12:16

what's with the magic numbers? we're aiming to reduce the number of undefined integers rather than increasing them

SirkoZ · Post by **SirkoZ** » 20 Jun 2005 17:53

Jango wrote:we're aiming to reduce...

Who are you anyway?

Look at the patch or rather patched industry_cmd.c + source and everything will be clear.

DaleStan · Post by **DaleStan** » 21 Jun 2005 04:00

SirkoZ, if you want this patch committed, use eg

Code: Select all

#define MEANINGFUL_NAME 5
#define MEANINGFUL_NAME2 10
//...
for (int i=MEANINGFUL_NAME;i<MEANINGFUL_NAME2;i++){
//...
}

instead of

Code: Select all

for(int i=5;i<10;i++){
//...
}

The latter uses so-called "magic numbers" which are horrors when trying to change or debug existing code.
0 and 1 as magic numbers are sometimes excusable.

bobingabout · Post by **bobingabout** » 21 Jun 2005 07:55

while the first takes up extra space... a simple comment stating what 5 and 10 mean are also good, but why use a dynamic number and take up extra memory for a number that never changes?

if its used several times, sure, a variable or constant would be ok, but if its just 1 or 2 times, why?

Asterix · Post by **Asterix** » 21 Jun 2005 09:07

bobingabout wrote:while the first takes up extra space... a simple comment stating what 5 and 10 mean are also good, but why use a dynamic number and take up extra memory for a number that never changes?

if its used several times, sure, a variable or constant would be ok, but if its just 1 or 2 times, why?

It does *not* take up extra space, because on compile-time those names will be automatically replaced with the corresponding values by the compiler.

And even if its only used a few times, it makes the code easier to read for those that didn't write it. (on a sidenote, I dont use enough names, myself

)

Jango · Post by **Jango** » 21 Jun 2005 09:22

Asterix wrote:
bobingabout wrote:while the first takes up extra space... a simple comment stating what 5 and 10 mean are also good, but why use a dynamic number and take up extra memory for a number that never changes?

if its used several times, sure, a variable or constant would be ok, but if its just 1 or 2 times, why?
It does *not* take up extra space, because on compile-time those names will be automatically replaced with the corresponding values by the compiler.

And even if its only used a few times, it makes the code easier to read for those that didn't write it. (on a sidenote, I dont use enough names, myself )

written by a true coder

- anyone who has tried to understand different sections of the ottd code will know that the sections that have been rewritten to use defines rather than meaningless numbers are MUCH easier to read. The main advantages arising from that are that it's easier to avoid bugs and it's quicker to code changes.

Whilst comments are good, defines are often subtler and slightly less verbose although just as meaningful. Having code littered with pointless comments often makes it more difficult to read.

Code: Select all

#define MEANINGFUL_NAME 10
y = (x < MEANINGFUL_NAME) ? 4 : 5 // 4 = something, 5 = something else

By using defines, you abstract away the numbers and concentrate on the actual code. With comments you leave the numbers there and they can be distracting.

Sirkoz wrote: Who are you anyway? :Shocked:

Clearly someone with more manners than you. I've done my fair share of OpenTTD coding as well, and I'm merely helping you to write code which you'll understand in a few years time. Besides, if you submit a patch like that, Tron will probably kill you. It's all very well having clever code (using bitwise operators and the like), but in reality it's MUCH slower to read.

GoneWacko · Post by **GoneWacko** » 21 Jun 2005 09:25

Just a bit of a pre-emptive warning:
Let's keep it civil

Mek · Post by **Mek** » 21 Jun 2005 10:56

DaleStan wrote:SirkoZ, if you want this patch committed, use eg
Code: Select all
#define MEANINGFUL_NAME 5
#define MEANINGFUL_NAME2 10

don't the coding guidelines say to use enum instead of #define? so

Code: Select all

enum {MEANINFUL_NAME = 5, MEANINFUL_NAME2 = 10};

Thief^ · Post by **Thief^** » 21 Jun 2005 11:10

in actual c/c++ coding practice enums should only be used when a variable can take more than one value representing different states.
As the example is:

Code: Select all

#define MEANINGFUL_NAME 10 
y = (x < MEANINGFUL_NAME) ? 4 : 5 // 4 = something, 5 = something else

y should be an enum type with "something" and "something_else" defined as 4 and 5, but MEANINGFUL_NAME is a constant, not a variable state (it's never assigned to x, only compared to it) so should be a #define or const int (and indeed is).

EDIT: btw, before you ask who *I* am, I'm a student who's just finished the second year of a BSc Games Programming course, in the middle of writing a custom full-3d physics engine, and preparing for an interview with a games company for my placement year.

Mek · Post by **Mek** » 21 Jun 2005 11:29

Thief^ wrote:in actual c/c++ coding practice enums should only be used when a variable can take more than one value representing different states.
As the example is:
Code: Select all
#define MEANINGFUL_NAME 10 
y = (x < MEANINGFUL_NAME) ? 4 : 5 // 4 = something, 5 = something else
y should be an enum type with "something" and "something_else" defined as 4 and 5, but MEANINGFUL_NAME is a constant, not a variable state (it's never assigned to x, only compared to it) so should be a #define or const int (and indeed is).

i was refering specificaly to this part of the wiki:

Code: Select all

 
*  Use simple enums for holding weakely related constants like this: 
enum {
  NUMBER_OF_BIRDCAGES    = 10,
  MAX_BIRDS_PER_BIRDCAGE = 3,
}
...
* Use enums instead of #define

(i'm not saying i think this is the proper way to do things, but this is just what is writting on the wiki

Thief^ · Post by **Thief^** » 21 Jun 2005 11:32

Fine, if that's what they say to do, but it's not strictly correct is all.
As long as there's a coding standard it'll be fine, (which there is

)

Jango · Post by **Jango** » 21 Jun 2005 11:37

and, bringing us back to the point, either way is better than magic numbers

SirkoZ · Post by **SirkoZ** » 21 Jun 2005 12:34

Thank you for all of your constructive criticism - I have changed the code to use predefined values (now in industry.h within an enum statement).

Here it is "screenshot" from industry_cmd.c:

Code: Select all

if ((i->pct_transported[j] > PERCENT_USED) && CHANCE16I(1, CHANCE_USED, r)) 
                new += ((RandomRange(INCREASE_A) + INCREASE_B) * old) >> 8; 

if ((i->pct_transported[j] < PERCENT_USED) && (i->pct_transported[j] > 0) && CHANCE16I(1, CHANCE_USED, r) && (old > MIN_RATE)) 
                new -= ((RandomRange(INCREASE_A) + INCREASE_B) * old) >> 8; 

if ((i->pct_transported[j] == 0) && CHANCE16I(1, CHANCE_UNUSED, r) && (old > (MIN_RATE + 1))) 
                new += INCREASE_UNUSED - (RandomRange(MIN_RATE));

And a "screenshot" from industry.h:

Code: Select all

enum { 
	CHANCE_USED = 50,
	CHANCE_UNUSED = 10,
	INCREASE_A = 50,
	INCREASE_B = 10,
   INCREASE_UNUSED = 2,
	MIN_RATE = 5,
	PERCENT_USED = 153
};

SirkoZ · Post by **SirkoZ** » 21 Jun 2005 13:24

Here are the latest diffs/patches with the above code - from nightly/svn source 2466 (patches for industry_cmd.c and industry.h are separate...):

Jango · Post by **Jango** » 21 Jun 2005 13:28

the "screenshots" look much better now

SirkoZ · Post by **SirkoZ** » 21 Jun 2005 13:34

Wow - thanks - well - the only "problem" is - it's now in two files, but I guess - it's OK - as long as it's more readable/understandable. I hope my integer's name choosing was somewhat right.

Please look at the diff-ed industry.h - is it OK, if it's at the end of file - that enum statement?

I have tested the modification and it works as before - as intended.

SirkoZ · Post by **SirkoZ** » 22 Jun 2005 19:17

Upon further, more extensive testing - this enum stuff doesn't perform as expected, so I'll remove all files from with it from here and SourceForge.

I read, that integers in enum statement are not always strictly used....

Please suggest some better methods...

EDIT: I used the wrong exe for testing - enum patch works OK.

Please disregard this post.

SirkoZ · Post by **SirkoZ** » 23 Jun 2005 12:06

There - this works alright - "screenshot" from industry_cmd.c:

Code: Select all

int CHANCE_USED = 50;
int CHANCE_UNUSED = 10;
int INCREASE_A = 50;
int INCREASE_B = 10;
int INCREASE_UNUSED = 2;
int MIN_RATE = 5;
int PERCENT_USED = 153;
                
new = old = i->production_rate[j]; 
        
if ((i->pct_transported[j] > PERCENT_USED) && CHANCE16I(1, CHANCE_USED, r)) 
           new += ((RandomRange(INCREASE_A) + INCREASE_B) * old) >> 8; 
        
if ((i->pct_transported[j] < PERCENT_USED) && (i->pct_transported[j] > 0) && CHANCE16I(1, CHANCE_USED, r) && (old > MIN_RATE)) 
           new -= ((RandomRange(INCREASE_A) + INCREASE_B) * old) >> 8; 
        
if ((i->pct_transported[j] == 0) && CHANCE16I(1, CHANCE_UNUSED, r) && (old > (MIN_RATE + 1))) 
           new += INCREASE_UNUSED - (RandomRange(MIN_RATE));

EDIT: Please - disregard this post.

Diadem · Post by **Diadem** » 23 Jun 2005 13:19

Why are you using int's for global constants. An int costs memory, it's a dynamic variable used for storing dynamic stuff. Numbers that chance.

You should use enum's or #define's for constants. Enum's are prefered in the coding style we use. If they don't work for some reason, like you claim, then use #define statements.

SirkoZ · Post by **SirkoZ** » 23 Jun 2005 14:19

I must have used the wrong exe - default one - I have now retested the "enum" patch and it works flawlessly. Sorry for confusion.

Here are the "enum-patches":

Transport Tycoon Forums

Improved smooth_economy patch

Who is online