• Frost (unregistered) in reply to Irrelevant
    Irrelevant:
    snoofle:
    Alex:
    Anyone else reckon it's output from the C preprocessor copy-pasted to production code?
    "reckon"?
    reckon.

    As in, "One hopes the guy who wrote this will have his day of reckoning?"

  • Ch0 (unregistered) in reply to Robert
    Robert:
    There are those of us who believe code should be self-documenting. I tend to favor Markp's approach.

    I'm not allowed to write self-documenting code.

    Well I am, but I have to comment it too.

    I had to change:

    echo "Enter username:"
read username

    To:

    # Read the username
echo "Enter username:"
read username

    I wish I was joking.

  • rmatt (unregistered) in reply to Matt R

    Yes, that's right give all constants near meaningless names, but put a comment near the declaration. That's the way to go.

  • BA (unregistered) in reply to rmatt

    Can I be the first to ask "What the F" is this amount of time being used for?

    Because all of the variable names and options I've seen so far suck hard balls.

    What about something closer to (assuming the variable is used to time the delay between automatic saves) the variable's intent rather than a description of its contents.

    final autosaveDelay = 5 * 60 * 1000;

  • Eric Jablow (unregistered) in reply to Matt R

    You should use

    final long ms = 5 * 60 * 1000L;

    It doesn't matter here, but it would if you had to use a longer period of time.

    final long ten_days = 10 * 24 * 60 * 60 *1000;

    won't work, because the product is of ints, and overflows.

  • (cs) in reply to Matt
    Matt:
    jk:

    and wasn't it the ancient Irish who decided the alphabet should be in alphabetical order?

    You just blew my mind man. By definition wouldn't the alphabet be in alphabetical order?

    Not if you're Irish.
  • Nate (unregistered) in reply to Matt R

    You're both wrong.

    const int c_MILLSEC_PER_SEC = 1000; const int c_MILLISEC_PER_MINUTE = c_MILLISEC_PER_SEC * 60;

    final long ms = 5 * c_MILLISEC_PER_MINUTE;

    Relying on comments to make a line even remotely understandable is wrong, ok?

  • Mr. V (unregistered) in reply to Anon.
    Anon.:
    // Just in case the number of seconds per day is changed. #define SECONDS_PER_DAY 86400

    You are all wrong. Considering the fact that when you use UTC clock not all your minutes are equally long (some are longer or shorter than others), the number of seconds in a minute/day/year changes.

    And just because of that some days are actually one second longer (or shorter)!

    As the number of seconds in a minute changes, you should obviously create a macro that calculates it and not a #defined constant.

    You are all the same self-centered bastards as the bunch that does: #define DAYS_IN_YEAR 365

  • kalix (unregistered) in reply to Matt R

    I prefer the 5 * 60 * 1000 route. Any compiler (yes, even java) will insert 300000 into the reulting code.

    Verified by decompiling a class ;)

    k

  • nibh (unregistered) in reply to Matt R

    [quote user="Matt R"][quote user="Markp"]

    Or gasp you could use a comment! Then you can remove the assumption that the moron who will be reading/maintaining your code will see the "obvious".

    final long ms = 300000; /* 5 minutes worth of milliseconds */[/quote] Or better yet, name the little buggers. final long SECOND_IN_MS = 1000; final long MINUTE_IN_MS = 60 * SECOND_IN_MS; final long ms = 5 * MINUTE_IN_MS

  • Spikefu (unregistered) in reply to Matt R

    Right, and then when the moron updates the code, but not the comment you'll have something like this:

    final long ms = 3000000 /* 5 minutes worth of milliseconds */

    instead of

    final long ms = 50601000;

    Yeah, definitely better to use comments to convey meaning than make the actual code self documenting.

    As for the original one liner, I think the developer was smoking crack when they wrote it.

  • BillyBob (unregistered)

    I think this is a fairly obvious one.

    It used to be:

    patno -= 40 + 10;

    Then some idiot went a did a global replace of 10 ==> VERSION_CODE

    and for some reason adding 1 every year fixes the problem, so it evolves into:

    patno -= ((((((((((((((((40+1)+1)+1)+1)+1)+1)+1)+1)+1)+1)+10)+1)+('Z'-'A'))+1)+1)+1);

  • BillyBob (unregistered) in reply to frustrati
    frustrati:
    SuperousOxide:
    APH:
    Actually, I would combine all these suggestions:

    final long TIME_IN_MILLISECONDS = 5 * 60 * 1000; // 5 minutes in milliseconds

    I know myself, and I would definitely omit or add an extra zero.

    Or you could go further

    final long TIME_IN_MILLISECONDS = 5 * SECONDS_IN_MINUTE * MILLISECONDS_IN_SECOND;

    You never know when someone will redefine the minute or millisecond.

    You clearly misunderstand the purpose of defining constants.

    final long FIVE_MINUTES = FIVE * SIXTY * ONE_THOUSAND;

    You jest but I saw this in code just last week (to be fair it was ONE, TWO, THREE.... TEN).

    The way I to it is:

    final long WAIT_PERIOD = 5 * MINUTES_TO_MILLISECONDS;

  • BillyBob (unregistered) in reply to Loren Pechtel
    Loren Pechtel:
    Sure, they can never change but they're still magic values.

    Have read this a few times here and I don't understand the logic. Seconds per minute, e, pi, are all things that will never change but I'll set you on fire if you don't use a constant for them.

    By using constants instead you make it clearer what the code means.

    indeed.

  • Loren Pechtel (unregistered) in reply to BillyBob
    BillyBob:
    Loren Pechtel:
    Sure, they can never change but they're still magic values.

    Have read this a few times here and I don't understand the logic. Seconds per minute, e, pi, are all things that will never change but I'll set you on fire if you don't use a constant for them.

    By using constants instead you make it clearer what the code means.

    indeed.

    It's a matter of clarity.

    You see final_time = 5000 and you don't know what it means. Even if you saw final_time = 5 * 1000 you still don't know what it means.

    Is the fundamental clock unit milliseconds or something even smaller?

    Sure, in the final_time = 5 * 60 * 1000 there's no question because we recognize there is only one set of conversion factors that would make this make sense. Just because you can reasonably get away with it once doesn't mean you should, though--simply get in the habit of always using descriptive constants and you won't get burned.

    The clearer the code the less trouble it's going to be down the road. I've still got a year or so of supporting stuff I started 20 years ago.

  • Simon (unregistered) in reply to Matt R
    Matt R:
    Markp:
    I do this:

    final long ms = 5 * 60 * 1000;

    .

    Or gasp you could use a comment! Then you can remove the assumption that the moron who will be reading/maintaining your code will see the "obvious".

    final long ms = 300000; /* 5 minutes worth of milliseconds */

    And when you need to change it's value you gasp have to write a program to help you calculate the new value in milliseconds. I would first check the oxygen level in your cubicle...

    Simon

  • Joe Batt (unregistered) in reply to Matt R

    That's fine until it becomes

    final long ms = 600000; /* 5 minutes worth of milliseconds */

    Joe

  • questionable_taste (unregistered) in reply to Matt R

    What are comments? What possible use could something like this have??

  • ZZP (unregistered) in reply to Matt R
    final long ms = 300000; /* 5 minutes worth of milliseconds */

    Unnecessarily complex.

    • You make the code less obvious by introducing the magic value 300000.
    • This forces you to add a comment to explain it.
    • The comment will likely become out-of-sync with the code at the next update.

    The GP is right, this is better: final long xxx = 5 * 60 * 1000; // ms

    • The unit is documented
    • No redundance code/comment
    • Pretty obvious it is 5 min
  • phantomcircuit (unregistered) in reply to Matt R

    except then it's more difficult to change the number of minutes....

  • Robert Kosten (unregistered) in reply to Matt R

    That has only one serious drawback: When someone changes the constant without changing the comment... It'll confuse the heck out of the next one to come across it (I do a lot of jump-in-and-fix-it, so I've seens this embarrasingly often...)

    My personal favourite way to write something like this is: final long SESSION_TIMEOUT = 5 * 60 * 1000; // specified in ms

    Where I have: a) descriptive variable name b) upper case variable name, so it's obviously a constant (depends on your style guidelines and language, of course) c) one place to change the value, which is presented in a readable way (and any compiler will optimize this away, so nothing to worry there) d) comment that tells me what order of magnitude to expect here.

  • Sam (unregistered) in reply to Dave

    If this was Java code then it would be inefficient, you want to use StringBuilder then convert it to string buffer.

    new StringBuilder("Nic").append("e.");
  • DribbleDropper (unregistered)

    I see a lot of people trying to add even more WTFness, but on the serious side the cleanest solution is this:

    const int millisecond = 1; const int second = 1000millisecond; const int minute = 60second; // etc. Perhaps use double instead of int.

    // All code using times must explicitly state the unit, e.g. const int some_delay = 5*minute;

    After all, we're dealing with units. It's pointless to have seconds_in_milliseconds etc., because then you will still need to know which of seconds and milliseconds is the fundamental unit (i.e. the one used internally).

  • (cs) in reply to DribbleDropper
    DribbleDropper:
    I see a lot of people trying to add even more WTFness, but on the serious side the cleanest solution is this:

    const int millisecond = 1; const int second = 1000millisecond; const int minute = 60second; // etc. Perhaps use double instead of int.

    // All code using times must explicitly state the unit, e.g. const int some_delay = 5*minute;

    After all, we're dealing with units. It's pointless to have seconds_in_milliseconds etc., because then you will still need to know which of seconds and milliseconds is the fundamental unit (i.e. the one used internally).

    There you go again, dealing with sanity and rationality instead of plain good ole one-upmanship.

    That will never work on a site like this.

  • Guy (unregistered)

    I'd guess this was part of a default password, and he was going for a 'Y', not 89.

  • Kevin (unregistered) in reply to Matt R

    final long ms = 300000; /* 5 minutes worth of milliseconds */

    I would say this is wrong. If you change the time, you have to recompute the milliseconds. Also duplication of information is bad, you are likely to change the value and forget to change the comment.

    final long ms = 5 * 60 * 1000;

    Requires no such comment (or a comment could simply be /* in milliseconds */, and is easy to change the number of minutes.

  • John Atkinson (unregistered)

    Hahaha, look at the brackets go as you scroll down.

  • 008 (unregistered)

    If patno isn't an int, the operators + and - could be overloaded to do something else(like add or remove to/from a collection) and return the math result.

  • Kuba (unregistered) in reply to Dan
    Dan:
    LISP would have been better suited to this problem. change patno -= ((((((((((((((((40+1)+1)+1)+1)+1)+1)+1)+1)+1)+1)+10)+1)+('Z'-'A'))+1)+1)+1);

    to

    (+(+(+(+(+(+(+(+(+(+(+(+(+(+(+(+(+ 40 1)1)1)1)1)1)1)1)1)1)1)10)1)(- Z A))1)1)1)

    now it's readable, and there's one more pair of ()!

    ROTFLMAO. What were ya smoking? Here's LISP:

    >(+ 40 1 1 1 1 1 1 1 1 1 1 1 10 1 
    (- (char-code #\Z) (char-code #\Z)) 1 1 1)
65

    Looks nothing like the abomination you posted above. I guess clueless posts are never out of fashion.

    (Disclaimer: I'm a LISP clueless-post fascist. So what.)

  • Lyle (unregistered) in reply to wm.annis
    wm.annis:
    Honestly. 
    (1+ (1+ (1+ (+
(blah blah blah)
                           (+ 40 1))))))))))))
             10
             (apply #'- (mapcar #'char-int (list #\Z #\A)))))))

    Ekhm, why complicate things so much?

    1. Addition (and other common operators) take multiple arguments, so there's no reason to manually apply increment (1+) so many times.

    2. List constants can be written in 3 characters less.

    3. Using char-int (or even char-code) twice would take way less space than as you have done with apply and mapcar.

    Lyle

  • Me (unregistered) in reply to Matt R

    ..and then what if you later need to change it to 6 minutes worth? 5 * 60 * 1000 is easier to change, no?

  • LKM (unregistered) in reply to Matt R

    Writing 5601000 is better than writing 300000 and commenting why it is 300000. If you write a comment and change the number later, you may forget to update the comment. The next person looking at your code may not be sure if the comment or the code are supposed to be correct.

  • jaymz (unregistered) in reply to Matt R
    Matt R:
    Or *gasp* you could use a comment! Then you can remove the assumption that the moron who will be reading/maintaining your code will see the "obvious".

    final long ms = 300000; /* 5 minutes worth of milliseconds */

    i'd personally rather do it the OP's way and spell it out so to speak. You might as well have it calculate it itself and it makes changing the number of minutes much easier since you only have to change the 5. If you had to use 4.389 minutes or something, writing 4.389601000 is less prone to error than manually writing 263340 /* 4.389 mins of ms*/

    i think anyway :)

  • akozakie (unregistered) in reply to Matt R

    Yes, because it's so much easier once you're told to increase that time by one minute. Instead of changing one number, you have to change one number and a comment, and you have to calculate in your head. 300000 is easy, but what about 13 hours in seconds? Easy - yes, but why bother? Besides, sooner or later someone's going to ignore the comment while changing the constant.

    final long delay = 46800; /* 14 hours worth of seconds */

    Good luck finding this bug. It's actually better to use both self-documenting constants and comments or even just the constants exactly because of the moron who will be maintaining your code..

  • (cs) in reply to mh
    mh:
    Anon Fred:
    #define SECONDS_PER_DAY 86400 #define FIVE_DAYS 5 * SECONDS_PER_DAY
    #define SECONDS_PER_DAY 86400 #define FIVE_DAYS (5 * SECONDS_PER_DAY)

    ;)

    Ahh... somebody who's read Code Complete? Nice catch

  • Monkey Brains (unregistered)

    I was immensely entertained by the number of people who consider changing 5 minutes in milliseconds to 6 minutes in milliseconds a difficult challenge prone to errors.

    Thank you for that.

  • A Lisper (unregistered) in reply to Dan

    Rubbish. Real lispers know about n-arity functions and would write this:

    (+ 40
   1
   1
   1
   1
   1
   1
   1
   1
   1
   1
   10
   1
   (- (char-code #\Z) (char-code #\A))
   1
   1
   1)
==>
89
  • Jimmy (unregistered)

    Many years ago, i worked with a C compiler for the Infineon E-GOLD chipset that would have loved the code in the article.

    After many weeks of debugging weird behavior it turned out that it only evaluated the first 2 variables during variable declaration, so :

    int a = 1 + 2 + 3;

    would equal 3.

    Of course you always "skip" through variable assignments while debugging and go straight for the breakpoints :(

  • DiD (unregistered) in reply to Matt R
    Matt R:
    Markp:
    Moitah:
    i'd say the guy wanted to keep a trace of each time he had to increase the reducing value.

    Exactly! Who needs CVS?

    Anyway, there's nothing wrong with writing out what should be a constant in an expression to make it's semantics clear. When I want to specify 5 minutes in ms, I do this:

    final long ms = 5 * 60 * 1000;

    not this:

    final long ms = 300000;

    So from reading the expression, it's pretty obvious what the semantics of that decrement are.

    Or gasp you could use a comment! Then you can remove the assumption that the moron who will be reading/maintaining your code will see the "obvious".

    final long ms = 300000; /* 5 minutes worth of milliseconds */

    It's a lot easier to change when someone wants 6 minutes instead of 5 if it's using Markp's method. Saves having to use a calculator every time you need to change it. You could add a comment as well though.

  • Comment Nazi (unregistered) in reply to Matt R

    Cept you wouldn't want to use /* */ on a single line comment in case someone needed to comment out the block later :p.

  • magicNumber (unregistered) in reply to Matt R

    Or: final long MILLISECONDS_IN_5_MINUTES = 300000; Of course, someone else would have gone for : final long MILLISECONDS_PER_SECOND = 1000; final long SECONDS_PER_MINUTE = 60; final long MINUTES_TO_CALCULATE = 5; final long I_AM_ANAL_RETENTIVE = MILLISECONDS_PER_SECOND * SECONDS_PER_MINUTE * MINUTES_TO_CALCULATE;

  • Chris (unregistered) in reply to wm.annis

    Really, you guys don't know lisp. + isnt restricted to two arguments.

    (+ 1 1 1 1 1 ... 1 1 1 (- \Z \A) 1 1 ... )

  • (cs) in reply to Chris
    Chris:
    Really, you guys don't know lisp. `+` isnt restricted to two arguments.

    (+ 1 1 1 1 1 ... 1 1 1 (- \Z \A) 1 1 ... )

    And an implicit cons to you too. Any particular flavour of Lisp you're thinking of?

  • Anal Retentive (unregistered) in reply to magicNumber
    magicNumber:
    Or: final long MILLISECONDS_IN_5_MINUTES = 300000; Of course, someone else would have gone for : final long MILLISECONDS_PER_SECOND = 1000; final long SECONDS_PER_MINUTE = 60; final long MINUTES_TO_CALCULATE = 5; final long I_AM_ANAL_RETENTIVE = MILLISECONDS_PER_SECOND * SECONDS_PER_MINUTE * MINUTES_TO_CALCULATE;
    Hmm, yes, I see. Renaming MILLISECONDS_IN_5_MINUTES accordingly when the required number of minutes changes, recomputing its value, then doing a global search and replace of the entire codebase to update any references to the old constant is indeed a far superior approach.
  • Simmo (unregistered) in reply to jk
    jk:
    Alph:
    This already happened -- in the Greek alphabet, Z comes right before H. ("zeta" and "eta"). The Romans didn't use Z, so when they took the alphabet they replaced it with G. ("But wait", you say, "doesn't Greek have their own G ("gamma") right at the beginning?" To which I say, "Shut up unless you want me to write a 10 page essay on the history of the alphabet.") Later on, the Romans decided they wanted Z after all, so they just stuck it on at the end.

    I think the Queen (I guess; it's her English, isn't it?) should tell us all to add Þ back to our alphabet, probably at the end, in which case you will not be laughing when all this programmer has to do is change "z - a" to "Þ - a".

    ("Þis programmer", I mean. And "I Þink Þe Queen", too, I guess.)

    greetings from Iceland, where the thorn has been in use for a thousand years.

    and wasn't it the ancient Irish who decided the alphabet should be in alphabetical order?

    heh. Greetings from Australia. And actually I think it should be "I Þink ðe Queen". Or we could get even more phonetic and use the IPA, but I can't be bothered to be more of a smarty pants than this

  • Crazy (unregistered)

    #define CRAZY_CONSTANT 89 /My special number/

    patno -= CRAZY_CONSTANT

  • Errno (unregistered) in reply to Matt R

    final long ms = 300000; /* 5 minutes worth of milliseconds */

    Then you've got 2 things to update every time you change it and you have the possibility of the 2 getting out of sync.

    I'd rather see Markp's solution, provided that you're using an optimizer that reduces the expression into a constant. It's also nice to define constants for conversion like MS_PER_MINUTE.

  • Paolo G (unregistered) in reply to Simmo
    Simmo:
    jk:
    Alph:
    [snip]

    I think the Queen (I guess; it's her English, isn't it?) should tell us all to add Þ back to our alphabet, probably at the end, in which case you will not be laughing when all this programmer has to do is change "z - a" to "Þ - a".

    ("Þis programmer", I mean. And "I Þink Þe Queen", too, I guess.)

    greetings from Iceland, where the thorn has been in use for a thousand years.

    and wasn't it the ancient Irish who decided the alphabet should be in alphabetical order?

    heh. Greetings from Australia. And actually I think it should be "I Þink ðe Queen". Or we could get even more phonetic and use the IPA, but I can't be bothered to be more of a smarty pants than this

    I thought that too at first, but then remembered that Þ was used in "the". That's how we ended up with "ye" as in "Ye Olde Tea Shoppe" - printers didn't have Þ in their typefaces, so used "y" instead (although I don't know why they didn't use "p", which would have been closer in appearance). Now you can impress all your friends by telling them that "ye" (meaning "the") is pronounced the same as "the"!

  • JustAGuess (unregistered)

    89 is used for the seed of a very secret algorithm. And of course no one should be able to read the compiled source ;-)

  • Chainsaw (unregistered)

    There's always the possibility of stupid, but..

    I'd be willing to bet the only thing REALLY wrong with the code is that they didn't use constants and/or better naming conventions for all the conditions they're correcting for...

    It may or may not be the case that encoding the whole thing as a single constant is compatible with the rest of the code. If there are lots of modules with different numbers of off-by-one issues, it may be time to refactor the whole program, but you aren't going to fix it by writing this function better.

Leave a comment on “A Dubious Honor”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article