current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I have a PRNG-Algorithm:

/**
* Generates an integer. Min: 0 | Max: total - 1
* @param    total The number of possibilities.
* @return The generated number.*/
public function generate_normal(total:int):int
{
    _seed = (_seed * MULTIPLIER) % MODULUS;
    return (_seed * total / MODULUS);
}

The constants are defined as follows:

/**The Multiplier.*/
private const MULTIPLIER:int = 48271;

/**The Modulus.*/
private const MODULUS:int = 2147483647;

This is readable and understandable, but pretty slow (~50% more time).

Simply using the numbers would be faster, but less readable. Will people call these numbers "magic" and kill me or will everything be fine? Should I change this part in the class description:

Modulus suggested 1988 by Stephen K. Park and Keith W. Miller.
Multiplier suggested 1993 by Park and Miller.

to this:

Modulus (2147483647) suggested 1988 by Stephen K. Park and Keith W. Miller.
Multiplier (48271) suggested 1993 by Park and Miller.

Should I keep my approach or use the numbers directly?

TL;DR What's more important, performance or readability?

share|improve this question

2 Answers 2

up vote 1 down vote accepted

There is no one answer.

Performance

How often is generate_normal called? Is it called on a program critical path or only during set up?

Could you cache a bunch of numbers before hand and only replenish the cache when you run out?

Readability

How many people will read this code over its lifetime? How often will you have to come back to the code?

Compromise

If you notice a substantial penalty using the private const construct, then do the following:

/**The Multiplier. Suggested 1993 by Park and Miller.*/
private const MULTIPLIER:int = 48271; // folded into code below

/**The Modulus. Suggested 1988 by Stephen K. Park and Keith W. Miller.*/
private const MODULUS:int = 2147483647; // folded into code below

...

public function generate_normal(total:int):int
{
    // Values folded from definitions ...
    // _seed = (_seed * MULTIPLIER) % MODULUS;
    _seed = (_seed * 48271) % 2147483647;
    // return (_seed * total / MODULUS);
    return (_seed * total / 2147483647);
}
share|improve this answer
    
Performance: I'll be using it mostly for games, about 5 times per frame; nice idea with the cache. Readability: I think that at least I'll be sharing it with some people who help me with the games. –  gonida May 10 '14 at 14:35
up vote 0 down vote

Readability:

>>> hex(2147483647)
'0x7fffffff'

Using a hex constant would be way more readable.

Performance:

Honestly I can't believe that using the numbers would be faster. Do you have any measurements to support the claim?

share|improve this answer
    
I just made a little test: I stored the current time in milliseconds, tested with constants (25.000.000 iterations) and stored the current time minus the time before. Then I did the same again without constants. The times varied alot, but this was one run: With constants: 3398 | Without constants: 2211 | With constants took 53% more time. –  gonida May 12 '14 at 12:04

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.