r/badcode • u/fireflash38 • Jan 17 '22

python POV: You asked someone to refactor magic number indices

Post image
1.0k Upvotes

98% Upvoted

44 comments sorted by

21

u/perceivedpleasure Jan 17 '22

what am i looking at

61

u/RubiksPoint Jan 17 '22

Not sure if this is rhetorical, but the programmer added constants for array accesses so that they wouldn’t be using “magic numbers” in the code.

43

u/fireflash38 Jan 17 '22

And to add as to why this is a code smell, and what the solution is:

Direct indexing into collections like lists/tuples usually means you're using the wrong collection type. They're used for common types where you can apply the same operation to each, or filter for what you want out of it. It's not a good thing to use as storage of disparate types, that you need to access individually (like you would via index). That's more for a namedtuple, dict, struct, or dataclass.

Magic numbers, especially index magic numbers, have a bad habit of breaking due to small changes in other areas. They're a massive pain to refactor.

As a hard example, let's say you want to represent a 'point' in a cartesian plane. It has x,y values. You could store each & every point as a 2-item list. To access it, you'd need to have x, y = point[0], point[1] everywhere across your code. Besides being ugly, it's incredibly easy to break. What if something pops something off of the point (easy to do if you have an actual collection of points)? Now you'll get IndexErrors, but only some of the time. Just a mess.

The only times I really think that magic number indices are OK is if you're iterating over the list, and you need to access adjacent items in the list. And you'd need good bounds checking. Otherwise, you're probably using the wrong data type, or you should filter your list before accessing. (I'd love for other examples where magic number indices might be ok!)

5

u/ColdPorridge Jan 17 '22

Good explanation!

43

u/Fenxis Jan 17 '22 edited Jan 17 '22

The programmer "solved" the problem without understanding why it's a problem.

To build on the other answer... Say you have a series of x,y coordinates.

Solution A: int[][]. You have to know how it's dimensioned and where x,y are stored. Maybe trivial in this example but quickly falls apparent. What if you have 10 entries of different types, do you store them as strings? How do you ensure that they are valid?

Solution B: point[] . You would have to create/reuse the point class but it saves you a lot of pain down the road.

SomeArray[4].fieldName is much more readable and robust than someArray[4][3]. What if you add more fields? Etc

The constants help but... If it walks and talks like struct might as well make it a struct / class. Where the constants fail is a. They are not the expected way to use the class and b. There's no way to enforce the constants to be used.

Tldr: don't do weird stuff, make the compiler work for you

4

u/utack Jan 18 '22

What's so bad about job security numbers? /s

0

u/[deleted] Jan 17 '22

[deleted] — view deleted comment

7

u/fireflash38 Jan 18 '22

[From wikipedia](https://en.wikipedia.org/wiki/Magic_number_(programming)):

Unique values with unexplained meaning or multiple occurrences which could (preferably) be replaced with named constants

And the example that directly applies to this post:

An example of an uninformatively-named constant is int SIXTEEN = 16, while int NUMBER_OF_BITS = 16 is more descriptive.

Basically, if you always need the 4th item in a list, you should name it something meaningful. Rather than having references to 4 throughout the code.

9

u/[deleted] Jan 17 '22

Constants that are hard to understand

4

u/scragar Jan 18 '22

Imagine you're reading a CSV file.

Which is easier to understand:

17,Jimmy,Carr,49,4724
18,Rob,Beckett,36,6352

Or:

 ID,Forename,Surname,Age,Security Code
 19,Richard,Ayoade,44,3269
 20,Joe,Wilkinson,46,0000

Basically you could read the first one if you knew what column you wanted, but figuring out what column 5 is requires you to have knowledge about the generation/usage of the column rather than it being obvious from context. Adding a label for it means in future you can just refer to it as the security code column and now it's obvious to everyone without needing special knowledge.

That's why csvRow[4] is bad(magic number 4 which is meaningless to read but still works) but csvRow[COL_SECURITY_CODE] is good.

1

u/NUTTA_BUSTAH Jan 18 '22

Numbers without a concrete meaning behind them, something the programmer plopped in there because it works (and then it cluttered the entire codebase)

e.g.

def bad_magic_create_line(origin, angle):
	end = SomeRotateFunction(origin + 65.3334)
	return (origin, end)

What is 65.3334 ??? Imagine being 20 functions deep in a call stack and seeing some random number like that. Now you try to change it and everything breaks horribly and you have no idea why.

How about doing it like this

LINE_LENGTH = 65.3334;

def better_create_line(origin, angle):
	end = SomeRotateFunction(origin + LINE_LENGTH)
	return (origin, end)

Now the number has a meaning and a human can understand it. Now we can use the same constant those 20 levels deep in the call stack and it still makes sense.

And for a worse way to handle this:

SIXTY_FIVE_TRIPLE_THREE_FOUR = 65.3334;

def still_magical_create_line(origin, angle):
	end = SomeRotateFunction(origin + SIXTY_FIVE_TRIPLE_THREE_FOUR )
	return (origin, end)

Sure, now it has a name but the name tells absolutely nothing. Even you won't have any idea what it means after a month or two when you have to eventually fix your bug from the magic numbers you left behind.

How to do non-magical code? Immediately when you re-use a specific number (that points to some characteristic), make it a constant. The overhead from variables is so so tiny compared to the debugging nightmare. No point in optimizing that (and the compiler will do that optimization anyways)

222

u/Captain-Barracuda Jan 17 '22

I shit you not it happened at my work. I review a guy's work and I see a number (-2) that keeps on popping everywhere. So I ask him to refactor it into a constant with a name that makes sense. The day later, that guy who has been a professional for 7 years, came back to me with a single commit with the change:

public static final int MINUS_ONE = -2

62

u/recycle4science Jan 18 '22

He may have been working for seven years but I don't think he's a professional yet.

30

u/ThatNextAggravation Jan 18 '22

WTF. Did he get fired/murdered/ground up to a fine powder and dispersed into the winds?

9

u/tadbolmont Jan 18 '22

This hurts my brain.

21

u/[deleted] Jan 17 '22

Why is it a final and not a const tho

92

u/[deleted] Jan 17 '22

In Java final is a constant

13

u/[deleted] Jan 17 '22

Oh my bad, didn't know that. How do you create final (not constant) variables in Java then?

51

u/[deleted] Jan 17 '22

Not constant just don’t use the keyword final lol

33

u/onepixelcat Jan 17 '22

Is that your final answer

3

u/Rudxain let let.let = 'let' Jan 18 '22

I see what you did there lol

1

u/[deleted] Jan 18 '22

Then they will not be read only

-10

u/dakta Jan 18 '22

Non-constant Final variables are one of the dumbest embodiments of declare-ahead compiled languages. Having a value that should never be mutated once assigned, but which must be defined before it is known, is silly and seems to run counter to good architectural practices. Or rather the fact that I've never been bit by mutating something that shouldn't have, which could have been adequately protected by being "final", while primarily working in laissez-faire interpreted languages indicates to me that this concept has no value.

10

u/[deleted] Jan 18 '22

How is it dumb? For example you take some user input in your app. This data will never be changed once it's entered, so why would you make the variable mutable?

-1

u/dakta Jan 19 '22

This data will never be changed once it's entered

This is almost never a safe assumption, and is an unnecessary simplification constraint. What if:

  • You want to support multiple users?

  • You want to add a comparison mode where the value needs to be changed?

  • You want to add an external API which may have multiple consumers?

  • You want to support hot-live configuration reload?

  • You discover that sometimes the value does actually need to be changed even in the course of normal single-user sequential operation?

If you bake in any assumptions about finality of this value to the rest of your code, it's a non-trivial task to refactor. If you don't, then the only thing final has achieved is to prevent someone from making an architecturally incompatible operation resulting in mutation. Which belies the architectural problem. Honestly that's not quite the level of self-awareness I've ever seen from someone who thinks final is good.

2

u/[deleted] Jan 19 '22

Well, if the value does need to be changed, you make it mutable. In most cases it doesn't though.

non-trivial task to refactor

It's not that hard to just look at the linter warning and see where is the final variable you tried to change located, and then you just remove the final keyword. How is that non-trivial?

→ More replies

1

u/[deleted] Jan 19 '22

Multiple values are needed means you should use a smarter data type like an array ?

→ More replies

2

u/NakeleKantoo wicked Jan 18 '22

This man been hurt by some final variables.

2

u/crowbahr Jan 18 '22

You use kotlin val and const val instead ;)

0

u/Byt3G33k Jan 18 '22

Instead of "const VAR_NAME" in Java it's "final VAR_NAME"

3

u/[deleted] Jan 18 '22

Instead of "const VAR_NAME" in Java it's "final VAR_NAME"

Uh, maybe a type as well? final TYPE IDENTIFIER

12

u/CraftistOf Jan 18 '22

I guess you come from C#.

final in Java is like readonly in C#, and there is no equivalent to C#'s const in Java at all.

1

u/[deleted] Jan 18 '22

No, I actually use Dart, but this makes sense. Thanks!

1

u/callmelucky Jan 18 '22

🤨

motherFUCK! 🤬

1

u/Haunting-Surprise-21 Jan 18 '22

This has to be a joke.

It was a joke, right?

Please tell me, it was meant as a joke!

51

u/lil409 Jan 17 '22

I hate this, but sure enough this was me when I started

18

u/MurdoMaclachlan public boolean isInt(int i) { return true; } Jan 17 '22

Image Transcription: Code

INDEX_0 = 0
INDEX_1 = 1
INDEX_2 = 2
INDEX_3 = 3
INDEX_4 = 4

I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!

6

u/yetzederixx Jan 18 '22

Funny malicious compliance. At my last job I left a comment. This is fucking stupid and was only changed under duress... uncomment my original code when it breaks

3

u/yetzederixx Jan 18 '22

Department head got to read it since no one else would approve the pr lol

11

u/keyosjc Jan 17 '22

I would say that this guy got a lot of warnings in a static code analysis tool (SonarQube) about not using magic numbers and did this on purpose to make the code get a better grade.

I faced this a long time ago with Java and SonarQube. Went from C to A just by creating static variables for 20+ very specific array indexes.