- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Admin
For the first one, you are a bit of a WTF. If you want to make sure that other developers realize that 3600 is the number of seconds in an hour, there's this really neat new invention called "comment".
For the second one, the minor WTF is that the code doesn't limit the upload size to 20 MB. Instead, because of the call to round, the limit is actually either 20.5 MB or 20.5 MB minus 1 byte (I'm not sure offhand what PHP's default rounding method is).
Admin
I don't agree at all. If you can make the code self-documenting, do that instead of comments. Comments has this habit of not being updated along with changes to the code.
Admin
I don’t think there’s much of a WTF there. I’m all for increased readability and making your intent clearer. Makes the entire thing more maintainable, no?
Also at least the second one is probably optimized out by the compiler anyway, so there’s not even a (teensy tiny in the first place) performance impact.
Admin
Agree with Dennis, and if it occurs frequently and you don't want to write 60*60 each time, this is a good place for a "magic number" constant such as THREE_THOUSAND_SIX_HUNDRED=3600 ;)
The second one may be intentional (trying to be generous here), the logic being that if a user has a file of 20.3 MB and their file manager shows it as 20 MB (rounding), the user would be surprised if the server rejects it. But if that's the reason, it should be documented, if only to prevent the next programmer to "fix" it and thus break it.
Admin
Certain fairly fundamental and recognizable "magic numbers" are inevitable in coding. Days per week, minutes per hour, hours per day, 1024 for Ki, etc. Derivative magic numbers like 86400, 1440, 10800, 1048576 are just as real, but not as recognizable. Something like:
reads pretty obviously as a week's worth of milliseconds. Wheras
does not. With intermediate levels of clarity for intermediate derivative constants like
7 * 86400000.I'd only use the
* 1idiom if there were other parallel cases nearby in the code that used* 2or whatever. Imagine anelseifchain for comparing obj.Prop <1 hour, <2 hours, < 3 hours, etc. The* 1in the first case is visually consistent with the others and also implicitly answers the question "Did the dev who wrote this forget something?"Works for me. May not for you.
Admin
Ninny. What's wrong with SIXTY_TIMES_SIXTY?
Admin
Both examples cry out for that final value to be a constant where the name explains what it is/does. For example: HOURS_CUTOFF = $(1) MAX_SECONDS = $((6060HOURS_CUTOFF))
A bonus benefit is, should the requirements change to cut off at 2 hours you just change the constant value.
Admin
Actually, "... you just change the constant value" " should read " ...you just change the constant value after searching to ensure that declared constant isn't used anywhere else where the requirement there is NOT changing. And if you find any such instances, create a new constant for the new requirement and replace all the references in changing requirements to the new constant, while replacing none of the references in non-changing requirements.
In large projects prone to less than strict enforcement of scoping rules, you may find that constant has quite a bit larger life than glancing at a few lines of code might suggest.
Admin
I'm fine with 60*60 and am surprised to see others think it a WTF - but the *1 definitely made me do a double take. It only makes sense if at other times or other places in the code this would be a different multiplier, in which case as we all know a descriptive name for what the number of hours actually represents would be much better.
Admin
I think I've personally been in enough situations where it starts off as "the time interval for this is one hour" and then it turns into two, three, or a variable number of hours, so I always like to extract that unit out into its own term, even if I don't explicitly need to.
Admin
Only if declared as SIXTY_TIMES_SIXTY = 30 * 120
Admin
Yeah, remembering stuff sucks. If only we had a way to make the computer remember that for us. Like if we had a way to assign a name to that quantity, and the computer had some kind of memory in which we could put it.
That seems backwards, given that you're still going to have this code creating a variable to store the lifetime. I'd sooner pull 3600 into a constant and leave 1 alone. So you'd have LIFETIME_SECONDS = 1HOUR_IN_SECONDS - and if that's not clearly saying that the lifetime is an hour, I don't know how to help you. What would be the point of making LIFETIME_HOURS and immediately multiplying it by 6060 to convert it to seconds? And what happens when someone modifies one of the two variables after the conversion step, so now they don't even represent the same duration? It just makes the code harder to reason about for no benefit.
Admin
Great discussion on programming styles. Sad part is, we appear to be out of WTFs ;)
I suppose we'd have a different conversation if there was an array of constants for all the magick numbers and the code looked like this:
MAX_SECONDS = CONSTANTS[3] * CONSTANTS[8] * CONSTANTS[1]
Admin
When I was doing embedded work, we had several time constants based on the frequency of our timing circuit. So it would be totally normal to see something like 60 * ONE_SECOND.
Admin
Well, if the first one is supposed to be shell, then there are extra spaces around the
=, so all you'll get is "bash: MAX_SECONDS: command not found". Or am I missing something?Admin
It doesn't specify which shell. It's not necessarily Bash.
Admin
Today it was 24*1°C. I know the *1 is superfluous, but it makes it a bit more clear like this…
… No, it doesn't. It's a WTF if there's a need (either true or felt) to cater for a potential unawareness of units coming in 1's only.
Addendum 2022-11-01 11:38: (it's OK if the magic value 1 is made explicit by sth like DEGREES_CELCIUS_PER_DEGREE_CELCIUS though)
Admin
Even better spelt CELSIUS
Admin
Am I the only one confused by the comment? It says MB but 1024 * 1024 is a MiB...
Admin
Close, but definitely no cigar. It's PHP or JS, so the chances of it being optimised out by the compiler are approximately not even as much as zero. And quite frankly, as noted, the performance impact is tiny, especially compared to the upload time for whatever's being uploaded. Bandwidth available for uploads has increased hugely over the years, but there's still a lot of people with really crap ADSL service where one megabit per seconds == 8 seconds per megabyte is what you get if your house is in the telephone exchange, and for people who live in town, it's more likeha
Addendum 2022-11-01 12:42: like half or less of that, so the time for PHP or JS to calculate
1024*1024is irrelevant.Admin
Which in the article it's not, because the
*1is for how many hours, and the60*60is the unit conversion. It's not completely unreasonable to suggest that it would be the most explicit possible if it was written as:but I'd be inclined to specify the timeout as a number of minutes (i.e. in context
60) rather than a number of hours, and multiply it by a minutes-to-seconds factor:Admin
The comment is wrong, it should have been: // files must be less than 20.5MiB
Admin
I worked with some old electrical software. I kept seeing *1 a lot of places. Finally, I discovered a lot of places where there were *1.1 and *1.2. They were applying 10% and 20% fudge factors in their designs. It looked like garbage, but at least the reason eventually became clear.
Admin
The second example is a WTF. Because, the file upload is NOT limited to 20MB (however you define MB). Instead, the file size check is made AFTER the file is uploaded.
But yes, I have often written values like const maxDurationInMilliseconds = (12 * 60 * 60 * 1000) -- it's clear what the variable holds (a number of milliseconds) and what the value is (um, 12 hours).
Admin
The only WTF I would see is not naming the magic numbers. SECONDS_IN_MIN * MINUTES_IN_HOUR * MAX_AGE_IN_HOURS (or better an SECONDS_IN_HOUR * MAX_AGE_IN_HOURS where SECONDS_IN_HOUR is defined somewhere as SECONDS_IN_MINUTE * MINUTES_IN_HOUR) is clearer than 60 * 60 * 1, but both are clearer than 3600. I've seen them enough that I usually recognize the time-based magic numbers, but increased readability is better. I've also seen a few codebases where those basic conversion factors were conveniently stuck in a helper class somewhere (with actual helper methods!) so they were really clear.
Admin
Count me amongst those who don't have a problem with magic numbers that are obvious pieces of reality like these are--although I certainly would have put the 1024s on the other side of the comparison. Only in context, though--60 * 60 is fine if you're clearly looking at time, unacceptable if the line doesn't mention time.
Admin
Well, I'd like to know which shell it is then, because it does not seem to be csh, tcsh, ksh, or zsh either.
Admin
I've been coding since the 80s. If you can't remember 3600 seconds in an hour, do you deserve to call yourself a programmer? It's one of the most fundamental physical constants that probably least likely to change over the lifetime of any program, after things like Pi and e, but those aren't exactly easy to remember exactly. I mean, do you #include "myAsciiDefinitions.h" in every program that looks like this:
#define END_OF_STRING 0 #define START_OF_HEADING 1 #define START_OF_TEXT 2 ... #define NEWLINE 10 #define NEWLINE_AS_CHAR '\n' ... #define SPACE 32 #define SPACE_AS_CHAR ' ' ... #define DEL 127, JUST IN CASE?
In which case, everyone else will have to learn your convention for these things, instead of the standard buff[0] = 0; / buff[0] = '\0';?
And then "myCodingDefinitions.h"
#define MY_IF "if" #define MY_ELSE "else" #define MY_ENDIF "" ... #define MY_PRINTF printf ... ???
Admin
You know it, I know it, but it's almost a lost battle. I'd be more confused by a comment saying 20MiB and seeing 1000*1000 in the code.
Admin
I would say buff[0] = '\0' is putting the actual character in, and as such doesn't get more readable, as opposed to "knowing" that 0 is the null character (although hardly a big jump). I guess there's a line - 60 seconds in a minute - everyone should know. 3600 seconds in an hour - OK, that should be common knowledge, but some people may not have that memorised (not everyone thinks the same way). 86,400 seconds in a day - less so (ignoring the whole topic of leap seconds, daylight savings, etc.). You want your code to be easily read by a newbie fresh out of whatever educational institution has spat them out, and you shouldn't assume they'll make the connections as readily as you would.
Admin
The part that most annoys me in the first example is that the number of hours is slapped at the end. Honestly, should it not have been 1 x 60 x 60? I mean, it's one hour, which we convert to minutes by multiplying with 60, which we convert to seconds by multiplying with 60. And if you want millis, you slap one last x 1000 to the end. As for the name of the constant, it might be fine if it is used in the next couple lines and only in the next couple lines. If that is not the case, it is a very, very bad name.
Admin
The "1" is not the unit. it is the number. the 6060 is the unit. If you read it like that, it makes sense, and it makes clear where and how to edit it if you want 2 hours instead.
In addition to making it obvious where the magic number comes from, things like 60601000 also avoid typos. 3600000, 36000000 and 360000 look pretty similar, while 1000, 10000 and 100 are clearly different at the first glance.
As for naming them, yes, that's often better, but in some cases it looks even uglier, or you have the named constant MAX_MEGABYTES and still want to turn it into bytes.
Admin
"Back in the day we were smarter" is not a valid argument for anything.
And memorizing ASCII codes, as you suggest, is nice while typing things together on an ancient abacus with 5 bytes of RAM, but nowadays, we write code that is not supposed to be understood by the guru that conjured it, but by whoever on the team reads it for the first time. It's not for easing the writing, it's for the sake of the reader.
Also, '\0' should be clear to most, I agree. But memorizing the code for EOF is just wasting brain capacity, and manually writing out '\n' or '\r' is bad practice, which should especially be known to someone who comes from a time when operating systems made way more of a difference between how line breaks worked than nowadays.
Admin
With this one I disagree. These numbers may be magic numbers, but 60 and 3600 at least are so common, as is the convention of specifying times intervals as integer in milliseconds, that something like
should be perfectly clear. I am a bit more conflicted on
and generally prefer to include the "seconds or milliseconds" information in the variable name, since otherwise it invites factor-of-thousand errors. Though that depends on whether in the language at hand "time in milliseconds" is actually a thing.
By contrast
is... Well, clear enough, but it doesn't really increase clarity over
3600and by being more verbose may actually reduce clarity. It certainly does subjectively for me.Beyond time, it gets more tricky. As a Physicist (working as a programmer since a few years), if I see something called "energy" and "length" I assume "Joule" and "meters", unless explicitly specified otherwise or regulated by project-wide convention. If I write code that needs to assume a deviating unit, I'd usually include the unit in the variable name to ensure that it is clear at every site, where that variable/constant is used.
But knowing about the issues of units and different unit systems (note that not even the number of basic units is the same across all unit systems) and different conventions coming from different engineering domains even within SI units (e.g. using millimeters as basic unit of length) is domain knowledge. I'd expect that from a physicist, from a mechanical or electrical engineer, but not from a mathematician or software engineer.
Admin
And actually we saw Michael's example not so long ago on the front page: when he had to fix a bug that sometimes files a little bigger than 20 MB were uploaded, like 20.3 MB...
Admin
Calculation with dimensional numbers (= quantities with units, be them seconds, Joule, or whatnot) follows strict rules that can be automated. If your program is extremely simple, you can get away with naming your variables "timeout_s" and being careful. Otherwise, you should enforce the dimensional rules programmatically. IMO "extremely simple" means at most two arithmetic operations, if you can use an existing library (here’s one of multiple options in Python : https://pint.readthedocs.io/en/0.20/getting_started.html).
If you have to do serious dimensional calculation without access to an existing library (because your language is not popular around science circles or because you cannot have external libraries in your production environment), I say you should still hack together your own minimal version of it. (Source: I once lost two days to a bug in my implementation of the ideal gas law, which is a rather basic / simple equation.)
Something along the following lines: your base class holds a value and a unit (V,U). V is a float number of whatever flavor you, your language and your use case like best. U is a multiset/bag that maps units to their (possibly-negative) exponent (for instance, speed is {m -> 1, s -> -1}; see the semantics of Python’s collections.Counter). (V1, U1) ± (V2, U2) throws an error if U1 != U2 and returns (V1 ± V2, U1) otherwise; (V1, U1) * (V2, U2) returns (V1 * V2, U1 + U2); (V1, U1) / (V2, U2) returns (V1 / V2, U1 - U2).
That is a simplified version. This disallows adding kilometers and miles (which you might want to be able to do) and it allows adding together two room temperatures in Celsius (which is meaningless from a physics point of view). But you can make it work in ~2h tops, and it protects against nasty bugs.
Admin
I've done something similar and would disagree with R3D3 that
is clearer than
First option, though short, has no indication of units, requiring enough familiarity to assume seconds which may not always be valid. Second option, while verbose but especially with WatersOfOblivion example of making the last value also into a constant like
MAX_HOURSclarifies it further.Returnee (after a long absence) mentions 3,600 should be simple enough to remember, sure. And if we needed milliseconds, just write it out as 3,600,000 or multiply it by 1,000 but this just makes it harder to read, especially for new developers, whether new to a project or new to programming in general.
Adding to what WatersOfOblivion mentioned about helper functions, for setting cache times in js I've created a function where you specify the magnitude and unit via simple chained functions like:
This performs the calculations when the unit type function (e.g.
hr()) is called, returning the value in milliseconds as needed for caching but is specified in a human readable manner. Performance being negligible as others have mentioned as you're more likely to have performance bottlenecks elsewhere.Admin
IIRC, an expression like 60 * 60 will be compile-time evaluated so it's author's preference as to whether it is more meaningful to a human reader than 3600; there's no runtime difference. On the other hand, getSize() / 1024 / 1024 > 20 has to do the divisions at run time where getSize() > 1024 * 1024 *20 again does the multiplication at compile time. That's over and above the matter of the logic breaking over at 22,020,096 vs 20,971521 .
Admin
In the second example multiplication is clearer than division anyway, because you don't have to think about the correct order of evaluation of operations.
(var / 1024) / 1024 > 20 is very different to var / (1024 / 1024) > 20
whereas, (20 * 1024) * 1024 is exactly the same as 20 * (1024 * 1024)
So the multiplication needs less mental load.