[-]

aa-b@reddit

Other dev sounds immature at best. This is a good opportunity to establish a style guide for the team, and get everyone to agree on that. "Avoid magic numbers" is an obvious thing to put in a style guide, and then you can just point to that to end the discussion

[-]

adjoiningkarate@reddit

Honestly surprised at the amount of people that don't have linters and failing builds on lint violations on this sub

[-]

FatefulDonkey@reddit

Because style is less important than security or blatant anti patterns (which magic numbers are)

[-]

adjoiningkarate@reddit

linters arent formatters. they enforce rules like max lines in a function/method/class, low cyclomatic complexity, rules for magic numbers etc

[-]

Salty-Wrap-1741@reddit

What tool and rule can you use to enforce 180 is not used directly in if statement? Or would it enforce no integer can be directly used but requires a variable?

[-]

adjoiningkarate@reddit

im mainly java so talking from checkstyle perspective, you can have rules for magic numbers, guessing possible on other languges linters too

[-]

Ekkmanz@reddit

Well-named function / validation that create context. This would made the magic number make sense.

[-]

Alborak2@reddit

This stuff is how you get crap like '#define ZERO 0'. Yes ive seen that in code.

Yes avoiding magic numbers is good, but there are times it hurts readability and adds another thing you have to look up.

[-]

oluwie@reddit

there are also rather times i’d rather change things one rather than in 3 different places, and then miss one because i didn’t know the number meant the same as all the other numbers

[-]

xelah1@reddit

The number of degrees in half a circle isn't going to change.

No-one uses latitude 0-180, though, it's always -90 to 90, at least on the Earth, so the original code is a bit weird to start with. Unless there's some context which makes it obvious it's begging for a much better explanation than a named constant.

[-]

StorKirken@reddit

Could be OP misremembering slightly, simplifying the example, or them using unsigned integers internally where latitude starts from 0 instead of -90.

[-]

TheMrBoot@reddit

Depends on where the code is operating at. Expected values could be 0..90 if you’re parsing out a position string that would be having an N or S attached to it.

[-]

premie_petey@reddit

And there are times you'd want to change things in one of three places only.

Most cases are clear cut in one way or another, but sometimes it's not that obvious.

[-]

ChypRiotE@reddit

Yeah this is malicious compliance and to be expected, but should also get caught during code review. Constant names should be explicit about what they represent. You wouldn't let a function named "doStuff" or variables named "a", "b" and "c" go through, same goes with a constant named ZERO

[-]

DanRunsOnRamen@reddit

This is what checkstyle (or your linter of choice) is for. Automate the argument away.

[-]

delphinius81@reddit

Yes, but the get everyone to agree to it part can be brutal. At some point, it cannot be a democracy and someone just declares brace style, variable naming convention, and tabs vs spaces.

[-]

npisnotp@reddit

A style guide is not a democracy, it is a dictatorship, and it should be because the focus is not "keep everyone happy" but "have a consistent style across the code base to reduce cognitive complexity when reading and writing code".

Of course that doesn't mean they can be created in a void, you should ask your team for feedback, but ultimately it will be imposed because, as you say, it's not realistic for all team members to agree on everything.

[-]

seeeee@reddit

Have a smaller group of leads agree and review, not the whole team. It’s still not a democracy, but you do want their feedback.

[-]

eightslipsandagully@reddit

Honestly this just ends up being the sort of pointless minutiae you waste hours arguing over for no real benefit. IMO just implement linting checks to enforce these things and move on

[-]

Minouris@reddit

Unprofessional and amateurish is what I'd call it. I wouldn't expect magic numbers from anyone above junior grade, and I wouldn't tolerate that level of sass in response to a review from anyone that level or below.

I let a certain amount of inelegance pass in code reviews, depending on level, as long as it's not buggy or ambiguous, but this guy would be getting the fine toothed comb of doom after pulling something like this.

[-]

coderemover@reddit

Absolutely not. The other dev expressed himself unprofessionally, but generally knowing when to apply rule like „no magic constants” and when not to is a sign of maturity. Blindly following some clean code nonsense from the internet is the sign of immaturity.

[-]

tenuj@reddit

Totally agree. The problem isn't that he refused to declare a constant for 180. Maybe he was right, maybe he was wrong.

The problem was how he acted.

Adding a mandatory code style will make most devs go insane, especially if those recommendations aren't even good. I don't miss the days of "generics must be one single capital letter".

[-]

vivec7@reddit

Style guide? I think Sonarlint may already have a rule for this. Don't even make it a discussion, make tooling the bad guy. Don't let them even hit the merge button until it's resolved.

[-]

seeeee@reddit

Do both, but yes definitely do linting checks first

[-]

Advanced-Owl-2513@reddit

try using dark mode to reduce eye strain durng late night browsing

[-]

Fryord@reddit

In this context, I would agree that 90 and 180 aren't magic numbers and it's more intuitive to keep them in the code directly, rather than extracting them. (in my subjective opinion)

Similarly, if you want to constrain an angle to [0, 360] (if using degrees), it would be more confusing to put use a variable Constants.maxAngle rather than 360 directly.

[-]

T0c2qDsd@reddit

I mean, the real move here is to exclude these values explicitly in the type system.

Angle is a type with values of either [0, 360) (degrees) or a value of [0, 2) (radians) (pick one or both, if you pick both include conversion between them and appropriate class level factory methods + private constructor), and you have to explicitly construct one. It’s an error to instantiate with invalid arguments, and if you don’t know if your argument is valid, you’d better call a class method “validate_…” first. :)

Then, the constants must live with the class and shouldn’t be used outside it. Problem solved! Anything internal dealing with angles gets an angle instance.

(I basically agree with others that this is a nit that I wouldn’t have blocked a PR on, but I would absolutely have blocked time to discuss harmonious code reviews if someone was that level of rude…)

[-]

rlbond86@reddit

The problem is there are some cases where you can have an angle greater than 360 degrees or less than 0. For example, "how far did the motor rotate in the last second" could be 720 degrees if it moves in two full rotations, or -720 degrees if it went backwards in two full rotations. I suppose you could call this an AngleDiff though

[-]

T0c2qDsd@reddit

Sure, although I’d probably recommend just having an explicit “rotation” and have it act on a directional vector to get a new one for something like that?

[-]

pigeon768@reddit

Angle is a type with values of either [0, 360) (degrees)

In some domains, the range is [0, 360) and in others it's (-180, 180] and in others for some fucking reason it's [-180, 180).

In my user facing code I accept [-180, 360], reject everything else, and have automated tests for all of the edge cases.

[-]

klimaheizung@reddit

"PR rejected, please rewrite project first into a sane programming language" ;-)

[-]

T0c2qDsd@reddit

I mean, this can be done in basically any modern high level language other than C. ;)

Performance implications and data locality may vary. :P

[-]

klimaheizung@reddit

Oh, I wish you were right. But not really. At least not if you want to do ANY kind of calculation. For example `if abs(lat) > 180` this requires that comparison `>` is defined for numeric (returned by `abs(lat)`) and the specific type that should be used instead of the plain 180.

Most languages can't do that. :(

[-]

T0c2qDsd@reddit

Operator overloading is potentially good if used sparingly and correctly? :P

But it should be something like:

Latitude.validate_input_degrees(lat_input); lat = Latitude.from_degrees(lat_input); // you never need to check lat ever again

[-]

klimaheizung@reddit

No, operator overloading is generally good if:

It's typesafe and wrong usage only ever causes a type error, no runtime error.
Semantics are intuitive (for most math operators, this is true)
It's clearly indicated that it is overloaded in the IDE at least (optimally also in tools like github), such as by changing color or similar and you can easily jump to it in the IDE
It doesn't introduce too much noise (which hurts readability more than it helps)

Unfortunately, it's not that easy to achieve those things all at once in most languages.

Latitude.validate_input_degrees(lat_input); lat =

No. You don't "validate" anything. It should be something like one of

lat = 180 degrees
lat = 180.degrees
lat = degrees 180
// already worse but still acceptable
lat = degrees(180)
lat = new Degrees(180)

[-]

T0c2qDsd@reddit

Yeah, yeah, with the caveat that /if/ you are doing input checking: ‘’’ lat = input degrees ‘’’ Will need to have well defined error semantics if degrees is outside the appropriate range, hence the validation calls I included. :)

[-]

deux3xmachina@reddit

Hell, it can be done in C with opaque types (like FILE *). You just need to write more code than you would with newer languages.

[-]

Lceus@reddit

Totally agree. Our solution uses latitude and longitude quite often (in context of addresses) and seeing "maxLatitude" instead of 90 would probably just confuse me. Only possible thing it could save is the mixup between whether it's latitude or longitude that goes to 180, but eh.

[-]

CpnStumpy@reddit

The thing is depending on the usage, the rational range could be -180 to 180, or 0 to 360, or -90 to 90 etc, the purpose of those max constants is that everyone can reuse them through the code and you never end up with a dev using metric when the whole system is built with imperial

[-]

kemitche@reddit

I think you're proving the opposite point here.

There's no universal "max latitude." What is a valid max could depend on the specific function and context, so extracting out to a universal constant somewhere means the 90 gets changed to a 180 when it really shouldn't.

It's hard to be 100% sure without all the code context, but here, I'd put a code comment rather than extracting to a named constant that specifically is NOT guaranteed to be reusable.

[-]

oldsecondhand@reddit

There's no universal "max latitude."

Yes, there is: the North Pole at +90 degree.

[-]

Lonsdale1086@reddit

Well yeah, but say you're adding a feature to allow dropping a pin at any place within your country, you might want to set the min and max to be the bounds of your country.

(Obviously you would need to check that the final coord is in bounds anyway so there's not too much value to this check, but it's just an example where the min/max wouldn't just be "anywhere on the globe")

[-]

rorschach200@reddit

Constants still don't help.

How would you call them if max could be 180, 360, or 90?

And how would you debug any given usage that a correct value is used in every use case?

With constants you have to analyze the usage, reference reference material to double check what the correct value needs to be, and the go to the definition of the constant to see if "MaxSomethignSomething" is hiding the correct value.

With the value as a literal you do the same minus the last step.

So if anything debugging became easier, and so is reading the code.

[-]

tommyk1210@reddit

The maximum latitude is 90 (north) and minimum is -90 (south). The maximum longitude is 180 (east) and minimum is -180 (west).

The value here in having these constants is comparison checking. Let’s say there’s a validateLongitude() method, if his comparison is to maxLatitude it’s clear something is wrong. If it’s if (long <= 90) then it’s much more difficult validate at a glance - you’re instead relying on everyone knowing lat/long max.

[-]

Sisaroth@reddit

I would argue that if you are writing code that deals with coordinates, you should already have a good grasp of how coordinates work. If you don't know how lat/long work I wouldn't want you working on that code.

[-]

tommyk1210@reddit

Not everyone works in a mapping business. Sometimes there might be a very small team working with coordinates or it might be a one-off feature.

I don’t see why it’s such a problem to make the code more readable, reduce the risk of setting the wrong limit in a conditional, and make it generally more tidy.

Sure, if you’re the Google maps team maybe you should know how degree coordinates work.

[-]

petrol_gas@reddit

Idk. I think the issue with the angles is that they are all relative. IF the “convention” info is encoded somewhere it should be in the function name and argument names. Probably object/property names too.

By the time you’ve gotten to using raw numbers, you should be in an abstract enough place that it’s perfectly clear what 90 means (because you’ve converted to a fixed reference point in a higher scope).

Regardless of the reference angle issue though, this discussion between the two of them and this Reddit post already cost more in man hours than the fix could have saved.

Preventing bugs saves millions? That’s if it ever does. And if we’re going to argue about whether it will or not with no context we’ve truly lost our minds.

I’d say poor form comment from OP, poor form response from coworker. Place sounds a smidge toxic to me.

[-]

rorschach200@reddit

That's what I find about most of reviews like this, that borderline ignore functionality (not a direct consequence of what is being described, but tends to correlate in practice), but spend hours, days, and in some cases weeks discussing stuff that perhaps not style per se, no, but hypotheticals and inconsequentials.

Looks, if you actually didn't understand the code, I get it.

If you understood the code, but sitting there speculating that some hypothetical other at some point won't, and the other person replying to you is making their own speculation on similar subject... I don't know guys, I think we are wasting each other's time here. Everybody means well, but we are spending more time on this than the best, least alone the average, decision of the two ever will save.

[-]

SecretaryAntique8603@reddit

But you don’t need to speculate on that - you can just assert that “writing X instead would make this more legible”, as opposed to “this needs to be more legible”.

If that’s true, there’s really no reason to speculate or bicker about it - just do it and save the energy.

Now, if it makes it less readable or worse in general you can object. But “I don’t think it’s necessary” is a really dumb objection to have if there is a valid improvement to be made.

[-]

rorschach200@reddit

> Now, if it makes it less readable or worse in general you can object. But “I don’t think it’s necessary” is a really dumb objection to have if there is a valid improvement to be made.

It depends, time is valuable. We are not here to sit and make every possible improvement we can possibly think of. We are here to be efficient and solve user problems as quickly and as cheaply as possible, and do so safely for all parties involved.

That's what good engineering is, "cheaply" is in the heart of it.

I generally use the rule of controlling your impulses during reviews. If something irks me, it's not a good reason to immediately write about it. Is this a big enough problem to be worthy of my time and the other person's time debating it? If not, move on. If yes, point out the issue.

Think about the other person, they are a mind of their own. There can be all sorts of things that irk me that doesn't irk them, and vice versa. We are not there to sync up our psyches to the same wavelength at all costs, we are there to solve problems. There is a real issue - report it. You got a twitchy eye - don't make that another person's problem (and customers - waiting for the delivery that takes longer now).

[-]

SecretaryAntique8603@reddit

I agree with that, it relies on some kind of common sense/good faith on the part of the reviewer. But if they point something out and you’re making changes anyway, it’s probably faster to just do it than to explain why you don’t want to and deal with the friction.

But if they keep pointing out inconsequential stuff on a PR that otherwise would have sailed through, then it’s a bigger problem.

I will mention small things like that sometimes, but I’ll make sure to point out it’s a nitpick they can ignore. If people can stick to that and use the appropriate level of insistence then this shouldn’t be a problem in a mature team imo.

[-]

Chocolate_Pickle@reddit

I would expect that geometric ranges/limits would definitely not be reusable.

These kinds of things tend to be mathematical requirements instead of convention/business requirements.

[-]

danielt1263@reddit

Is there a difference between metric and imperial degrees?

[-]

forgot_semicolon@reddit

Yeah, Americans use Fahrenheit/second, and everyone else uses radians/(time it takes for France to surrender)

[-]

cholz@reddit

There are different "units" to use for measuring angle yes. Assuming angle is always in degrees is just as bad as assuming length is always in feet.

[-]

evincarofautumn@reddit

The closest analogy is probably decimal degrees vs. degree–minute–second.

They’re both a mix of base 60 and base 10, but in different ways. This is because decimal sucks for divisibility. A “metric degree” should be 1⁄100 rotation, but now 30° – 60° – 90° becomes 8⅓% – 16⅔% – 25%. Have fun lol

[-]

Choperello@reddit

Yea. 90 degrees, 180 degrees aren't magic number. They're immutable constants. Like having a constant for the max # of degrees in a circle, or number of meters per kilometer. In what physics model are we working with that those things are things that can change? Etc.

[-]

Popular-Jury7272@reddit

Whether it's a magic number has nothing whatsoever to do with whether it's a constant.

[-]

Choperello@reddit

It really doesn’t. We also don’t do “int ZERO = 0” for the same reasons.

Because the reasons we do that is for readability, so future devs don’t look an wonder what the hell is 52646? And maintainability so we when we need to change “max timeout” we don’t have to change in 50 places.

Thats why we don’t use magic numbers, for those specific reasons, not some scary “all magic numbers are bad” mantra. Because there are some cases where the number isn’t magic and NOT using it actually fights against the original al maintainability and readability.

If someone sees a const for “DEGREES_90 = 90” the first thing that ought to go through their mind is “wait what what are the cases in which 90 degrees isn’t 90 degrees????” Or “kCircleDegrees = 360” the same question. SOME numbers are expected and make perfect sense, and setting up another layer of indirection decreases the goal of readability and maintainability.

[-]

True_Fig983@reddit

As a mathematician coder who frequently has to work with non-mathematician coders, I definitely agree that 90, 180 etc are NOT magic numbers that require a #define. They can be placed directly inline to the code and will add clarity, as other dev said initially.

I broadly support the use of #define constants generally and the moving of magic numbers to the top of the file or a header file with an appropriate constant. But when you are doing things like "mask >> 1" or "mask & 1" or even "i += 2;" there is clearly NO NEED to move these into #define constants. It is clear from the context what they mean (e.g. "i += 2" could be counting a loop that treats even and odd array entries differently, and if you don't understand that then a #define constant won't help).

Other dev's point is that when working with degrees, code like "angle < 90" or "angle -= 180" fall into the same category as my mask and loop counter examples. This is valid.

I think this points to some issues with peer code review generally: 1. The peer is often not a domain expert where the original dev is. That's why the job was given to the original dev rather than the peer. 2. The peer often feels they have to complain about at least one insignificant thing, just to prove they were paying attention. Possibly also feels the need to put their stamp on something, or feels the need to be an expert.

It used to frustrate me no end as both a mathematician and a consistency freak that I would go to incredible lengths to ensure that all notation is consistent, all variable names follow consistent naming schemes and all algorithms are well laid out with no unnecessary checks or operations. Then dumb ass peer comes along and requests various changes. I do not point out the inconsistency of his/her requests because that would be creating more work for myself and I would have to break more of my code in places he/she hasn't noticed. So to avoid arguments I just perform the minimum changes to make the reviewer go away. Then I look for opportunities to change it back next time I work on the code with a different reviewer.

The reason I follow this strategy is basically to avoid reactions like OP's reaction, where OP asked for something dumb and then got offended when other dev (domain expert) pointed out the stupidity of the request. Not worth the conflict or the bad feeling ...

[-]

kcrwfrd@reddit

That’s funny. I actually work on a mapping application and I would prefer a constant just bc i always forget which is -90 to 90 vs -180 to 180.

[-]

Strabro@reddit

I also deal with this at work. Easy way to remember is to think of your lats (back muscles). Similar Latin word origin equating to something like breadth or width. So the lines going around the globe width ways i.e imagine you're measuring the width of the globe with a tape measure. Then its easy, at least for me, to picture that there's less of these and that they go from -90 to 90.

[-]

Jaeriko@reddit

Then dumb ass peer comes along and requests various changes. I do not point out the inconsistency of his/her requests...So to avoid arguments I just perform the minimum changes to make the reviewer go away.

So you just didn't feel like justifying your design decisions and made more work for yourself by being passive? And then flip-flopped to sneak your choices back in under reviewers that won't press you? That is not a particularly glowing self report.

[-]

Kooky_Garbage9881@reddit

Nice work thinking you’re smarter than everyone. You don’t own the code, the team does.

[-]

Coxian42069@reddit

Perhaps it would help you to think "not everyone is as smart as me, and someday I might leave this job and they will have to manage my code, so making it easily parsable by lesser beings may be preferable to the utter perfection that is my methematical code-writing ability".

But in reality the greatest teacher will be the moment that you have to find a bug in someone else's code fairly urgently and it takes you an hour longer than it needed to because you have to mentally verify that every 180 and every 90 is appropriate in every place, rather than just seeing longitude < maxLongitude and moving on. "Wait, why were they using -180 to 180 here and 0 to 360 here??"

Who knows, maybe that lesson will come from having to parse your own code someday.

[-]

OldAd9280@reddit

It's very easily to accidentally use `if abs(lat) > 90` and that error might not be spotted, however `if abs(lat) > Constants.maxLongitude` would be more obviously wrong

[-]

opinionsOnPears@reddit

if abs(lat) > Constants.maxLatitude { ... }

Yeah, only reason I could see doing something like that is if the lax latitude wasn’t an obvious number like 90, 180, 360. Like, if you’re trying to see a max latitude of 150 or something.

[-]

ChaosCon@reddit

It ensures that you don't mix up latitude and longitude numbers by mistake. Abs(lat) > constants.maxLongitude should at least parse as odd to human readers. It's also a bit easier to step around for constants.maxLatitude without false positives I case the call sites need changing.

[-]

xelah1@reddit

This is where a thorough type system would be really handy, especially if you can distinguish units and conventions with it (longditude -180 to 180 vs 0 to 360 vs radians).

'lat = coordinates[0] # actually a longditude' is much more likely (both orders are used) and harder to spot, but geospatial and sciency types do seem to love representing data this way.

[-]

seven_seacat@reddit

It ensures that you don't mix up latitude and longitude numbers by mistake.

which is suuuuuper easy to do (not sarcasm, I worked on one app where I think i had to fix everything about four times over the course of the project)

[-]

xelah1@reddit

The naming is really breaking down here.

A 'max latitude' of 80 because the system doesn't work at the poles would be a reasonable constant or configuration value. A max latitude of 90 because that's a quarter of a circle is a different thing entirely.

The name isn't really distinguishing the two. 'High latitude cutoff' or some other shorter name might be better if it's not a quarter of a circle.

If it is the mathematical maximum and it's not 90 then I'd far rather see the number in the code, not buried in a constant. If it's pi/2 I'd really want that to be obvious. If it's 180, as in the post, then I'm really really going to want to know that, and why.

[-]

CpnStumpy@reddit

/r/mapswithoutnewzealand

[-]

SemaphoreBingo@reddit

What exactly do you think the latitude of New Zealand is?

[-]

MartinFrankPrivat@reddit

👆 this is the answer!

magic numbers a minor code smell, if that is the only issue, then let it be... arguing about such a small think is very childish and let's OP appear to be a control freak...

[-]

papa-hare@reddit

Yeah, I feel like using constants here actually makes the code confusing, because I'd assume there's something special there where it isn't. But the attitude of the other person either hints at this being straw that broke the camel's back out of their interactions or the other dev just being passive aggressive for no reason. But OP isn't right, in my (subjective) option either, his constants just make the code harder to read/reason about.

[-]

AnduCrandu@reddit

I was looking for this, I think using the actual numbers is plenty clear here and the original dev's way is fine. I don't think it's worth nitpicking details like this. But of course I agree that the dev's response was immature.

[-]

klimaheizung@reddit

I agree. Trying to extract them is a beginner mistake - it smells of someone living "by the book". Coding is an art and in this case, chances are the numbers are better not to be extracted. One of the questions can be "can this ever be changed" and "how often is it used" and "how easy is it to understand just by looking at the number itself".

That being said, looks like both implementer and reviewer are stubborn and impolite (esp. the implementer). Wouldn't want to work with either of them.

[-]

shozzlez@reddit

The GPT comment would send me into an absolute rage lol

[-]

Evening_Speech_7710@reddit (OP)

I was PISSED.

I wasn’t bothered to reply back to that though and just instead quoted him saying “Again there is no value to this”

And just replied with “Why?”

Nothing back from him and PR merged with the suggested changes.

But yeah that left a very sour taste in my mouth.

[-]

IronSavior@reddit

I'd save that away for performance review season

[-]

jasfour04@reddit

Legit what I was going to say. Keep the receipt and share with their manager

[-]

fucklockjaw@reddit

But how does that work? You wait X months and then go tattle on the guy?

Doesn't this just make you look bad?

[-]

IronSavior@reddit

It's documenting why he isn't ready for promotion. Absolute lack of maturity is unbecoming.

[-]

Qibla@reddit

Don't save it for performance review season though. Raise it now, and then remind at performance review season to see if there's been improvement.

[-]

IronSavior@reddit

This is definitely the most correct response for any place that has a (more or less) sane process.

[-]

Suspicious_Juice9511@reddit

Have you found such a place?

[-]

fucklockjaw@reddit

Yea exactly. What they're describing is an ideal environment but in my experience I tried going this route previously and I never came out on top.

Maybe it's because I don't have the 20 years of experience the commentor has (according to their flair) but talking poorly about my colleagues doesn't really end well.

[-]

IronSavior@reddit

It's in the framing. Don't talk bad about them. Talk about how their behavior impacts your work and the company.

[-]

fucklockjaw@reddit

I promise I'm not being sarcastic, honestly trying to learn here but I guess I have a difficult time knowing how to frame "they were belittling me in pr comments" and impacting my work and the company without framing it as kind of talking bad about a colleague.

[-]

Dacnomaniac@reddit

You get it, I feel like a lot of people in our industry don’t get this at all. It’s either some sort of personal attack or stay silent and seethe.

Gotta balance the personal responsibility/blame with work quality.

[-]

ChallengeDiaper@reddit

Almost is the correct solution. What u/shozzlez said before is important. Provide the feedback AND provide a solution on how to improve the org by suggesting a style guide.

[-]

enilcReddit@reddit

Wait…I’m confused…which person in the story is immature? The guy with the snarky comment or the thin-skinned guy whose feelings were hurt?

[-]

IronSavior@reddit

It's the bitch in the comments who can't tell the difference between a real grievance and being thin skinned.

[-]

causa-sui@reddit

You're not going to "tattle" because we're not children here, we're professionals. Being good at your job means not being a prick that everyone around you hates working with.

[-]

MCFRESH01@reddit

Yea I’d mention it in the next 1on 1 or bring up review etiquette issues in a retro

[-]

shozzlez@reddit

Yeah I don’t blame you. This might be where a team-wide “style guide” (or maybe an agreed upon CLAUDE.md nowadays) where you list things like “use declared constants for any fixed, constant values rather than inlining strings”. Then you can just comment with a link to the style guide.

[-]

sliding_corners@reddit

This! Because searching for 180 sucks but searching for max_lat is much better! For whatever strange reasons and max_lat needs to be set to 179, easy one line code update and its everywhere.

If it is in quotes, create a constant and thank yourself later!

[-]

coderemover@reddit

The thing is - comparing to 180 directly is way more readable than comparing to some named constant which has to be looked up to know the value. And in this particular case 180 is likely strongly tied to the formula it’s used in, it makes no sense to function outside of that context.

If you extract it to a local constant and use it only once, you’re just introducing noise. If you extract it to a global / module level constant you make it harder to lookup (ok smart editors help but it’s still one more click), but what’s even worse you’re probably introducing a hidden accidental dependency between different formulas that would likely also use the value of 180. But are you really sure the max longitude in module A formula is the same as in module B? If both use the same constant it suggests there is a logical dependency between them. However many times I saw how blindly applying DRY ended up in accidental dependencies.

[-]

Kaenguruu-Dev@reddit

To your argument about readability in the editor: You are arguing that seeing 180 makes the formula easier to understand but I disagree. Because if the 180 can be replaced by a max_latitude constant then the meaning of 180 is max_latitude and not 180 (if you get what I'm trying to say with that). So a new programmer seeing if lat > 180 will not immediately know what this check is actually ensuring in the business context, but seeing if lat > max_latitude is very easy to understand.

Now you might say "But if the two lines below are a log message and an early return it's obvious anyways" and while that is true, I don't think you can argue that having to read further into the code to understand a line higher up is a sign of good readability.

And knowing the exact value in that place only really becomes relevant when you are debugging the code in which case checking max_latitude once to see what the actual value is isn't really an issue. And nowadays I don't even need to click on anything I can just hover on it and it will show me the value. The time and effort really is minimal.

[-]

coderemover@reddit

No, the expression lat > 180 is perfectly understandable to anyone who knows what a latitude is. Anyone would immediately spot this is a bug :P. (Because latitude is -90..90)

[-]

Kaenguruu-Dev@reddit

But now you have asserted prior knowledge and experience that a junior developer (or any person for that matter) may not have and I don't see the argument of introducing scoped constants having such a big risk that it outweighs the easy imorovement that can be made.

[-]

ff3ale@reddit

Would you use HOURS_MAX instead of 12 or MINUTES_IN_HOUR instead of 60?

I mean you should be able to assume some knowledge, if you don't understand Latitudes and their ranges you'll probably won't understand the code anyway

[-]

LaughOk449@reddit

Yes i would definitely use HOURS_MAX, it describes that 12 is maximum hours

[-]

sidvinnon@reddit

Especially as we use 24 hour clock.

[-]

TheMrBoot@reddit

Right? Like, dude gave a perfect example of why it’s important without even realizing it lol

[-]

antifathrowaway1@reddit

Junior don't know to fear localization code. Intermediates see it as a challenge. Seniors cross themselves at the thought, then delegate all that work to Intermediates.

[-]

coderemover@reddit

Yes, obviously. Some prior knowledge is generally usually needed to understand code and there is nothing wrong with it. If someone doesn’t know how coordinates work, it’s on them.

[-]

Kaenguruu-Dev@reddit

Sure but we could simply make their lives easier with minimal work

[-]

coderemover@reddit

It’s not making it easier. If anything, hiding constants under names works only if the author of the code could come up with really good and unambiguous names. Otherwise it adds room for interpretation mistakes. Even the max_longitude is already ambiguous - because it can mean a physically maximum longitude that makes sense (180) but also a business requirement (maybe it’s only +25 because your area is restricted?)

[-]

Kaenguruu-Dev@reddit

Thats the point. Also we're not set on max_latitude you could name it whatever else something like max_allowable_latitude or whatever. But I don't think we have a chance of changing each others mind so I'll leave it at that

[-]

coderemover@reddit

Even if we agree there exist a good name, it provides very little value in this case. If the name replaces an entity that’s less complex than the name alone, it’s not very useful. And it’s even less useful if the meaning can be apparent from the context. I mean this:

if longitude > 180 { 
    return Err(“Longitude out of range”); 
}

doesn’t need any additional naming, it’s obvious. Hiding 180 behind the name makes it a tad less readable.

[-]

WearyCarrot@reddit

Damn so this is why my intro to Java professor made us comment all our variables huh

[-]

cach-v@reddit

Except you've kinda disproven your own argument. 180 as max lat is non-obvious given that latitude ranges from -90..90, not 0 to 180.

Plus, if you've never done any GIS work, you may not even know that.

Hence, constant and a quick explainer comment.

[-]

smootex@reddit

Is there a good source for good CLAUDE.md configs? I need to look into that stuff, the stupid AI code reviewer is getting on my nerves.

[-]

susmines@reddit

The docs provide good examples. My hot take is that extensively verbose instructional files do more harm than good in terms of token efficiency.

[-]

Minouris@reddit

Much. You're better off with focused rules at the start of each prompt that uses them. Skills lazy-load external resources as well, so if you have a validation skill that says "ensure that the rules in xyz.md are followed", it won't load that file into the context until it needs it :)

Basically, overloading the context with rules that are loaded at all times not only reduces the space you have for actual requests and relevant context, but it also dilutes the attention the model pays to each instruction, and it starts picking and choosing what to obey... Not good!

[-]

monkehh@reddit

A recent study has basically found that (with some caveats): https://arxiv.org/abs/2602.11988

[-]

Zeikos@reddit

Those files are a liability more often than not.
I find more reasonable accepting that LLMs make mistakes and check their outputs after the fact, as long as the code that has been written is deterministic it's easier and cheaper to check that.

[-]

Kaenguruu-Dev@reddit

Using a CLAUDE.md file does not free you from the responsibility of checking the code. The purpose of such files is to prevent waste of tokens because the AI keeps using the wrong naming conventions or similar.

[-]

rocketbunny77@reddit

;)

[-]

NatoBoram@reddit

This might be where a team-wide “style guide”

The issue for me with this is that this is such a basic software development basics that you learn in literally your first class of programming in school.

The concept of magic numbers and their effect on a codebase should be known as a baseline to every accredited developers and anyone with 3+ years of professional experience.

As in, if someone needs this to be in a style guide to do it, then they are of the most dogshit breed of developer possible and they should do something about their severe lack of knowledge and their piss poor attitude.

If you start accepting this kind of rule in the style guide rather than expecting it from the fact that they're a professional developer, then you're going to end up with a programming 101 handbook instead of a style guide.

[-]

ariiizia@reddit

Not a claude.md file. Just use a linter properly. You shouldn’t be able to commit shit like this.

[-]

SaltyBawlz@reddit

I would have blocked the PR until it was fixed tbh.

[-]

KhonMan@reddit

It was fixed, they just were sassy in the comments

[-]

reboog711@reddit

It sounds like the code was merged with the passive aggressive comments, unless I'm misinterpreting.

[-]

KhonMan@reddit

A comment in a code review isn’t the same as a comment in the code review

[-]

Antice@reddit

It's still a permanent part of the history if it is on github, and can be retrieved at any time in the future. The guy documented his own bad attitude.

[-]

reboog711@reddit

Agreed!

[-]

KTAXY@reddit

Sassy is just being insecure deep down. If you _really_ want to patch things up, you could try to engage with the guy at a deeper level (though they seem quite unpleasant person from this story). And explain to them that this attitude betrays their insecurity and they need to work on becoming a more adjusted, self loving person instead.

[-]

delphinius81@reddit

You could choose to ignore it and just move on. Or, you could nit pick the living hell out of the next pr they put up, and block merging until every style violation, no matter how inconsequential, is fixed. I expect a malicious compliance story in the future 😁

[-]

sahuxley2@reddit

Yeah, I'm with the PR dev. Nitpicking how much has to be explained/commented for the next dev is a bit much.

[-]

Dacnomaniac@reddit

Out of curiosity why exactly have you made a post talking about how annoyed you are not brought this up with the relevant party? Apologies if you have but it doesn’t sound like it from what I’ve read.

Bringing it up to them and framing it as not appreciating passive aggressive PR comments when you’re both contributing to the same goal is definitely something you should do.

It’s important to note there’s nothing wrong with resolving conflict when it arises and allows every dev to know what you expect of each other, and inversely makes sure it doesn’t feel like you’re being disrespected. It’s just a conversation at the end of the day.

[-]

v-alan-d@reddit

Darn it's too late to do this but next time try asking calmly and politely "what do you mean by that 'Nice usage of chatgpt'?" and let the burden of proof goes to them. And press on!

[-]

Minouris@reddit

"You're absolutely right!" ;)

[-]

WearyCarrot@reddit

Better to be more direct.

“@manager what does this mean????”

/s

[-]

pmkiller@reddit

He's right though. There are 2 schools of tought, you both failed to reach a consensus.

You're right to want to be able to ctrl+click see all usages of 90.

He's right that in his algorithm this should never change, so maxLat is wrong, feels changable by a new pair of eyes and will result in bugs or needing to understand the algorithm to just change that. Maybe it should be 90DegreesLat which sounds way too on point. So yeah just simple 90, 180 sounds just as good.

I say he's more in the right because extracting parts of the algorithm in random common constants just allows future coupling between unconnected features, while your way seems like a pet-peeve of yours. A common ground would be to have constants inside the algorithm rather than somewhere else.

[-]

PlasmaFarmer@reddit

You could comment on PR that you will link a wikipedia page about magic number - in case he missed it in school and career.

[-]

Chicken_shish@reddit

You need to remember that software development is not a democracy.

You are in a position reviewing the code. You want a change, you comment that change. The only discussion at that point is when it will be done.

Some devs are pricks, just like any other profession. You're right, he's wrong.

[-]

FactorResponsible609@reddit

Just escalate to manager 1:1 as Feedback for him..

[-]

PoL0@reddit

you rather constant names over magic numbers for searching purposes, plus having them defined in a single place. if you have coding standards there's probably an item about it. end of discussion.

his pov comes out as stingy for being called out on such a small thing. if this is their reaction to critique then you're in for a ride whenever you have to review their code

[-]

DAG_AIR@reddit

if you share a manager i'd raise "PR review friction" feedback with them, its up to their manager to correct this behavior.

[-]

Ok_Particular143@reddit

Never review their code again. Bad review experience means you move on to more important stuff first.

[-]

slopirate@reddit

This is one of those situations where you've just got to take the high road. That ChatGPT comment would have made me LIVID, but that was exactly what he was trying to do. So don't let him see that it upset you or that you even read it. You got him to make the change. You won. Take the win, and make a note about the snarky comment somewhere where you can find it later.

Your goal here is to document a pattern of disrespectful and unprofessional behavior on his part. Once you have 4 or 5 good examples, take it to his management. Say, "I really wanted to resolve this without coming to you, but this has become a pattern and I need advice on what to do about it. It's hurting the team's cohesiveness and productivity."

[-]

josetalking@reddit

For the future, the correct answer is "Thanks! :)".

Including the smily face, they were trying to get the up on you, which seems they accomplished.

Don't engage with rude or unprofessional speech.

[-]

Scared_Astronaut9377@reddit

Various conflict managing strategies may be more or less efficient depending on circumstances and goals. In many circumstances the strategy of just eating rudeness would be a way to become a doormat. While pettiness in PR comments means nothing, in-person rude behavior often needs to be met with immediate aggression.

[-]

josetalking@reddit

Agreed.

I am fortunate that there is only like 2 rude people where I work. I just avoid them like the plague, they don't pay me enough to become a therapist .

I will never understand the need to be rude or pejorative.

[-]

Scared_Astronaut9377@reddit

I think most of the time, there is certainly no need. People just have such characters. But, unfortunately, in many professional environments, being rude may help get promoted. The most typical example is interrupting people during important conversations.

[-]

JaneGoodallVS@reddit

I'd just let it sit without approving or replying, or screenshot it and reply to the screenshot

[-]

tryingtoescapereddit@reddit

Push back on entitled stupids. You should have replied not everything you don’t understand is from chatgpt but it’s good that you know about it, perhaps use it to learn about good coding practices :)) and you might see the value in it.

[-]

InfectedShadow@reddit

That would make me be so petty in any future reviews and nitpick every little thing.

[-]

Ok-Win-7586@reddit

Totally laziness by the colleague. Dude must be the life of the holiday party.

[-]

geekywarrior@reddit

[ Removed by Reddit ]

[-]

Probono_Bonobo@reddit

You're right to feel pissed off by this, and probably will for a while, but I promise you'll one day look back on this experience and laugh.

There's unfortunately a lot of toxicity in our industry, but as a general rule, toxic people aren't generally allowed anywhere close to the hiring process for fear of alienating potential new hires.

It sounds like a hiring manager decided to give you direct, unfiltered visibility into a day in the life on the team you'd be joining, rather than risking the possibility that you'd quit once you realized the culture was toxic. It's quite likely they've seen this happen before, maybe multiple times, and structured the interview so that you could see what you were getting into first.

I've had the reverse happen, where after a pleasant interview experience I've accepted an offer, only to realize that my coworkers are hostile and petty and have dark triad personality traits. It made every aspect of my life hell before eventually I realized that the only reasonable thing to do was to quit.

[-]

ButterflySammy@reddit

I'd have highlighted his comment and said what's the value in this?

[-]

airemy_lin@reddit

Style guide or whatever this other guy is just an assshole.

[-]

reboog711@reddit

Agreed; that is passive aggressive.

I have argued about using constants (or similar) for values used in a lot of places instead of literal numbers or strings. And I'm surprised it is even a discussion.

The best argument I've seen against this is that AI can easily update the values if needed, so keeping them in the constants no longer makes sense. I'm not sold on that yet...

[-]

officerblues@reddit

can easily update the values if needed

But you can't audit that without looking everywhere, and even AI tools will have issues with that as the codebase grows. Imagine, having to spend tens of thousands of tokens to update constants instead of just going to your single source of truth in one step. Then later on realizing that your AI also replaced the wrong constant and it slipped by everyone, causing an outage.

It's these small corners that hold everything together, don't cut them out.

[-]

ThingAboutTown@reddit

Absolutely this! If you sprinkle 90 and 180 through your code, how do you know when this 90 is the same as another instance of 90? If you have code that deals with latitudes and time delays for 90 seconds or 90 minutes, or strings that can be a max of 90 characters, how can you tell those apart?

Extracting magic numbers is absolutely coding 101 - anyone arguing against it shouldn’t be arguing about anything.

[-]

ff3ale@reddit

Do you use constants to for example define 60 seconds a minute before using it somewhere in logic? If you need to divide something by a fixed value, do you always define the divisor as constant?

There are plenty of situations where using a value is perfectly fine, where assuming the reader at least understands the code enough to understand what the number refer to.

Testing the logic for latitude bounds wont change nor will the max value for a latitude be used in any other checks. Just make a method if you're so concerned about missing opportunities for reuse, or make an latitude type.

What OP is suggesting is moving all unrelated numbers to a single place and that's somehow an improvement over having them close by?

[-]

ThingAboutTown@reddit

If I need to define some number of seconds or minutes, then I’ll define a constant like RETRY_INTERVAL_SECONDS = 5 * 60, and then use that wherever we need to use that duration. I’m not trying to avoid having to remember how many seconds are in 5 minutes, I’m ensuring that I can tell the difference between all the 5 minute intervals our codebase happens to have. If I want to know all the places RETRY_INTERVAL_SECONDS is used, that’s a no brainer. If I want to know which of the naked 300s represents the retry interval, that’s much harder.

[-]

reboog711@reddit

Agreed!

[-]

Agitated_Run9096@reddit

I replied explained my reasoning like self-documenting, thinking for future devs, less cognitive load etc… with examples on how it would look like.

I wouldn't infer it was passive aggressive, rather from OP's need to use "etc" and "examples" (plural) that OP wrote an unnecessarily verbose novel, to the point of being condescending, about how their style choice is better.

I imagine their first thought was, why did you write this everyone understands what constants are.

[-]

Due_Musician9464@reddit

But ai gets context from the constant’s variable names. This is false. Good names help ai as much as it helps people.

[-]

Vinegarinmyeye@reddit

I'd be angry with the "there's no value in that ".

I'd be fucking apoplectic with the ChatGPT comment

"The VALUE is that I'm not approving your PR until you do it, and when your manager asks you why the ticket is still open you can tell them the release is delayed because you were being a shitebag..." .

(There's a reason I don't work in corporate environments anymore. I don't suffer this kinda bullshit well... I wouldn't advise it, not very professional (but deeply satisfying the few times I have gone apeshit at someone who seserved it).

[-]

FatefulDonkey@reddit

That just makes you sound like someone with authority complex.

A good dev would actually sit and explain the actual value; avoiding truth drift, readability by users and LLM, LLM batch replacing, etc.

[-]

coderemover@reddit

If the reviewer has power to block a PR for such a stupid nitpick, then your work environment is toxic.

[-]

false_tautology@reddit

Someone else would approve

[-]

PlasticExtreme4469@reddit

I would just completely give up on caring what that person thinks or has to say. - No point in letting them affect my emotions.

[-]

oluwie@reddit

Not sure why we’re blaming the GPT here. The problem is clearly the dev.

Bad inputs always equals bad outputs.

[-]

PichovnaBertinova@reddit

Some guy told me the exact same thing... About a paragraph that I had written manually. Do I ask him if he would have had to use the ia to know such a basic thing... Because I haven't. He apologized next.

[-]

pacman6642@reddit

"nice use of chatgpt " after Claude wrote it

[-]

ModusPwnins@reddit

Yeah that's incredibly shitty

[-]

goatanuss@reddit

Id passive agressively install a linter that fails the build unless you fix magic numbers

[-]

mattcre8s@reddit

Install a linter that fails his code but only 30% of the time

[-]

EmmitSan@reddit

Putting shit like that in PR history is a massively stupid idea, above and beyond the pettiness of the comment in the first place.

[-]

officerblues@reddit

Oh yeah. That's asking for trouble. Even when I was a stupid, hot headed junior, I had the sense to complain directly to the person and not write it down like that. That guy must be really fun to work with.

[-]

SansSariph@reddit

I feel like the industry has always had a problem with collaborative respect, empathy and communication, and recognizing PRs and how they play out as a form of professional interpersonal communication... and AI is lighter fluid on that fire in so many ways

[-]

rlbond86@reddit

Everybody sucks here. Your coworker was too snarky, but your request to replace 90 and 180 are frankly stupid. Latitude always ranges from -90 to 90 and Longitude always ranges from -180 to 180. Making a constant in this case is not helpful, it's not a magic number.

This is like requesting someone replace the constant "3" when they have a routine that deals with triangles.

[-]

thesublimeinvasion@reddit

Longitude always ranges from -180 to 180

Well, unless you use unsigned longitude which means it ranges from 0 to 360... but yes, its OPs request that is stupid.

Also, it might not be a magic number, but it's not necessarily intuitive for all the developers that are going to be reading that code multiple times years down the line. And then you have the problem of mixing up the max and min values of longitude and latitude which half this comment section already has done.

[-]

qqqqqx@reddit

If their code says something like

if abs(lat) > 180 { throw error("Latitude too large")

then I would 100% say you're just being annoying because it's completely obvious. Making an extra enum for that seems excessive if that's the only usage. In some ways that adds cognitive load rather than taking it away.

If you want to say it, you should leave it as a nit and not block or insist on it. It's not nearly worth starting an argument or dying on a hill over.

[-]

thesublimeinvasion@reddit

Obvious? The max value for latitude is 90. You are just proving OP correct here by mixing up latitude and longitude max values.

[-]

qqqqqx@reddit

The example is literally copy pasted from their post brother.

Making it an enum doesn't fix the value being wrong.

[-]

thesublimeinvasion@reddit

If it was obvious you would have noticed the error, but you did not. Just take the L and move on.

[-]

bearicorn@reddit

That is not a magic number

[-]

gorankami@reddit

in my opinion, the numbers may be valid given a good argument and even though those numbers indeed are well known beyond swe professionals, but what actually happened is not a good argument to challenge your case on why to use constants.

In my experience seeing comments like this before, the way they used the argument was not to defend the code but rather to avoid work and its also done to undermine your case (i do not see them even trying to express that they understand what you meant).

My recommendation would be to record this (screenshot) in case its deleted. Then kindly bring up the topic 1-1 with whoever was passive aggressive to you irrelevant to the PR, but to point out how that behaviour in general is counterproductive to a professional environment and if you have more pushback, then bring it up to the manager/lead or whoever is responsible for the team.

Dont forget that this is in intentions to improve team work and morale rather than solving the PR

[-]

ThePersonsOpinion@reddit

Also, a junior, I too was confused by making global constants (for example, why do we need to use css variables/themes for backgrounds when we can just type "background: black"?)

The moment we had to uphaul our companies house-style and change every single CSS value line by line, I had my answer.

[-]

ThePersonsOpinion@reddit

What a Cun..- I mean, Const

[-]

Federal-Garbage-8629@reddit

Yeah! I don't know how this code review works. I feel it surely depends on your relations with reviewer. In my case, my PR was flagged as I used 404 instead Response::HTTP_NOT_FOUND though most of my colleagues get away with it. I saw this exact same thing during today's code review.

I would just say Thanks and finish the job. 😀

[-]

jrodbtllr138@reddit

It’s a valid nit comment.

I’d probably add the same, but make it non-blocking on the PR

[-]

dllimport@reddit

90 and 180 as degrees aren't magic numbers. What do you do when you need 45 or 60 or 30 or etc? They're going to be applied differently depending on the context. They're just trigonometric values.

I do hate magic numbers but this is a terrible example because these are just degrees

[-]

slopirate@reddit

I don't think you hate magic numbers enough, because those are magic numbers. Saying "these are just degrees" is not an argument.

[-]

dllimport@reddit

Maybe to illustrate what I mean why don't you suggest a replacement.

[-]

slopirate@reddit

A replacement for what? The whole point of not having magic numbers is that "they're going to be applied differently in different contexts". 90 degrees could mean a right turn, a left turn, a right angle, or a hot day. So you'd have

HOT_DAY_TEMP = 90
RIGHT_TURN = 90
LEFT_TURN = 90
MAX_LATITUDE = 90

That makes the code more readable by putting the semantic meaning in the variable name. That's the whole idea.

[-]

03263@reddit

Sorry but this is not DRY!

[-]

slopirate@reddit

Yes it is. Don't repeat yourself is about not needlessly repeating functionality, not having multiple variables that happen to have the same values.

[-]

dllimport@reddit

You aren't always going to be using it for those reasons. What if you're turning it 90 because it's in portrait mode. But in reverse portrait you need 270? And in reverse landscape you need 180? But then what if you have another dimension to consider so you have reverse portrait x and y but you need to move it -90 along the third axis? And then what if you have another adjacent and related use case that needs a a 45 degree turn instead of 90? Packaging the transformations into a named function makes sense, but naming every possible use for a 90 degree shift or a 270 degree shift for a particular use is going to get onerous and unmaintainable. Depending on your application you could easily end up with 100 enums. It only works for simple examples to name them. I am speaking from experience by the way and I am a HUGE fan of never having raw numbers in code. There are a lot of trigonometric systems where it is not not realistic or helpful to name them all. You should of course package the translations into something helpful like shiftToIsometric or whatever, but the underlying functions that those transformations use are frequently easier to both read and maintain if you just use the values themselves (in degrees or radians). Math is the name for them. We know that 90 degrees is a right angle. We know that -90 is a right angle in the opposite direction. Trigonometry already gave the relevant context that you typically get from using enums instead of raw values.

Obviously yes name HOT_DAY_TEMP. We are only talking about angles here, not anything else which of course you should use an enum for.

[-]

slopirate@reddit

> You aren't always going to be using it for those reasons.
That wasn't meant to be an exhaustive list...

> What if...
These are all reasons why you should always avoid magic numbers.

> Depending on your application you could easily end up with 100 enums.
And for large applications, we have 1000's. That's how you do it.

I do grant that there are you may want to just use the naked number (like when developing a trigonometric library), but those should be the extremely rare exception. So many bugs come from programmers assuming that something is obvious, or that a value won't ever change. The classic example is using "24" for hours in a day, which doesn't hold true during daylight savings time switches.

[-]

dllimport@reddit

Your example for 24 hours in a day is invalid for what I'm saying. I don't use magic numbers for basically anything except trigonometric translations. I agree with you that you should not use them for anything else

[-]

03263@reddit

const FORTY_FIVE = 45

[-]

divinecomedian3@reddit

Magic numbers are not a nit. They're a plague.

[-]

Empty_Expressionless@reddit

180 as max lat isn't a magic number. It's the definition of latitude.

Demanding an enum for that is only slightly more reasonable than asking for "min_num_apples" to replace the magic number 0.

[-]

magichronx@reddit

180 as max lat? I've only ever seen latitude range [-90, 90]...

[-]

IzzyD93@reddit

This completely proves why magic numbers should be avoided. I overlooked this completely when reading quickly. If lat > maxLong would be immediately noticeable.

[-]

Montaire@reddit

I overlooked this completely when reading quickly.

User error.

[-]

Dave4lexKing@reddit

Which is literally the whole reason for putting magic numbers in a named variable.

[-]

TheMrBoot@reddit

I don’t understand how so many people are walking headfirst into the point and not getting it

[-]

max123246@reddit

We automate machines to reduce user error. That's the whole point, because humans make many mistakes on repetitive tasks

[-]

guthran@reddit

Gottem

[-]

ACoderGirl@reddit

Agree, there's some numbers that don't need constants because they're obvious enough. For an extreme example, you wouldn't change idx / 2 into something like idx / arrayMidpointDenominator or whatever. It's admittedly contextual based off the domain, but there's some mathematical constants that just don't need a const.

[-]

pale_f1sherman@reddit

I would argue that the common value itself doesn't need a variable in a vacuum, but for example if the degree is used in a specific cases, then it becomes extremely useful.

Imagine something like:

LATITUDE_IF_PRODUCT_NUMBER_15 = 180;

or

DIVIDE_BY_WHEN_VAT_PRESENT = 2

Or something like that.

That way it becomes clear to others what the intent was behind a specific magic number. At least that's the way I see it.

[-]

thesublimeinvasion@reddit

So obvious that he got it wrong and you don't even notice?

[-]

xamott@reddit

Assuming that everyone knows that is obnoxious.

[-]

thesublimeinvasion@reddit

Yeah, and he even got it wrong. 180 is max for longitude, not latitude thus proving OPs point while being extra obnoxious.

[-]

Montaire@reddit

People programming lat / long systems really, REALLY should know it.

[-]

Empty_Expressionless@reddit

Do you vote

[-]

slopirate@reddit

It is absolutely a magic number

[-]

blorppppp_ttv@reddit

Pulling magic numbers out into constants isn't just about readability at the callsite.

It clarifies intent for future reviewers and helps w/ large scale refactors later.
Numbers aren't unique across domains, you might have a timeout of 180 seconds somewhere which has a completely different meaning. If we want to change the precision of our lat representation in the future, pulling the number into a constant makes this refactor trivial and clear.

[-]

Liskar-dev@reddit

I inherited a project that had a linter rule against magic values. I found constants like these in the code:

String EMPTY_STRING = "";
double DEGREES_90 = 90.0;

obviously made by clueless programmers just to make linter shut up

[-]

swiftmerchant@reddit

Sometimes it’s easier to use the editor to assign a string to EMPTY_STRING rather than “”. When you need to make a global search and replace EMPTY_STRING can be replaced quickly. I would not call the programmers clueless for doing this.

[-]

IBJON@reddit

Normally I'd agree, but it is pretty clear what those numbers mean based on context. I personally try not to make a variable for every mathematical constant unless its something less obvious or it's some esoteric piece of math that just really needs the extra info that a named variable can provide

[-]

Minegrow@reddit

This is absolutely not a nit lol

[-]

anubus72@reddit

it absolutely is lol

see the problem?

[-]

Minegrow@reddit

No, I don’t. You can disagree with anything, always

[-]

false_tautology@reddit

Yeah, mention as optional. Not worth the time to debate.

[-]

ais4aron@reddit

If you count that as nit/optional, it's never getting done. Have standards and work to them. Magic numbers are zero tolerance on my team.

[-]

false_tautology@reddit

They're apparently allowed in OP's team, so this becomes a political battle. He'll have to take stock of political capital an soft power. And, this is for a latitude constant, so I would simply advise not wasting anything on that.

If this is the biggest battle that needs to be won in the next year, I'd be surprised.

[-]

Fyren-1131@reddit

I'm guessing this is not about the constant.

[-]

false_tautology@reddit

Yeah, there may be history between these two. I've had my code called stupid in a comment and just laughed. OP seems over invested in this latitude constant for some reason.

[-]

Montaire@reddit

I had my code commented as stupid.

It was me. I was the one that made that comment.

[-]

false_tautology@reddit

Oh, I've surely insulted my code more than anyone else. I also have said I hate the guy who wrote this mess, who is also me.

[-]

sahuxley2@reddit

Faster to do it in your own PR than to argue.

[-]

vivec7@reddit

I generally consider optional to mean "if you believe you have a valid reason as to why this shouldn't be done, I'm not going to argue the point".

It has never meant "only if you feel like it" to me.

Either it gets done, or is responded to with a thorough explanation as to why I'm not going to do that.

[-]

mcmcc@reddit

Your code never has a naked zero?

[-]

jrodbtllr138@reddit

If nit/optional never gets done, that’s a culture problem.

My philosophy is if it doesn’t break, it meets the core requirements, and doesn’t cause harm to the product (security, performance, etc) a comment is rarely worth blocking the delivery since presumably the delivery would improve the product.

If you want to skip an optional ask, you make a ticket marked as tech debt as part of closing the comment and assign it to yourself.

We budget 10-20% for unexpected work, learning/exploration, improving product, and tech debt.

If someone has a mountain of tech debt assigned to them… it’s not a good look unless everyone knows they’re having high impact in which case, the debt is probably worth the impact.

[-]

vivec7@reddit

I generally consider optional to mean "if you believe you have a valid reason as to why this shouldn't be done, I'm not going to argue the point".

It has never meant "only if you feel like it" to me.

Either it gets done, or is responded to with a thorough explanation as to why I'm not going to do that.

[-]

will-code-for-money@reddit

Absolutely not a Nit, it’s extremely valid issue that in probably 99% cases should be rectified before merging.

[-]

An4rk-yy@reddit

The only correct response. Technically correct but gracious about it.

[-]

honestduane@reddit

No your colleagues are simply incompetent because the developer should know that magic numbers are bad and so if they don't think it's an issue that just shows that they're incompetent

[-]

PvtRoom@reddit

if you understand lat/long, you understand 90 and 180.

if you don't understand lat/long, what are you doing editing a lat/long function

[-]

soundman32@reddit

Why do you think its latitude? Variable is unclear so you need to dig further to work out context, which is bad.

From this snippet latitude is a float (or even worse, int), which is also terrible design

[-]

PvtRoom@reddit

constants.maxlatitude tells you that it's latitude. OP has no issues recognising that.

the only place in a sensible codebase where latitude should be anything other than a float or fixed point suitable for maths is parsing I/O, because you should be sanitising inputs and putting outputs in the correct format.

[-]

mental-chaos@reddit

My read of the situation is that OP makes nitpicky comments like this. The idea about avoiding magic numbers is not an absolute thing, but a continuum. On one side you have things like ZERO, NUM_ELEMENTS_IN_PAIR, NUM_SIDES_IN_SQUARE, on the other you have NUM_MICROSECONDS_IN_DAY.

Different people will have a preference that falls in a different spot on that continuum of when the constant helps the legibility vs just obscuring things.

That ChatGPT thing is probably not just related to this incident but to a pattern of similar interactions on PRs where you write them a lecture about code legibility rather than actually getting to the meat of the discussion. Something like "I always get latitude and longitude mixed up, having them be constants makes it easier for me to use the right one" in your pushback would probably have avoided that ChatGPT comment.

[-]

CthulhusPetals@reddit

Comment wording is nasty but I have real mixed feelings about this sort of thing. It’s a constant that will never, ever change. Putting “Constants.maxLatitude” in puts a cognitive load on the reader who then has to go read what that value is even though it will never change. That value is almost certainly going to be in a separate file requiring further work on the reader’s part. I feel I was taught in college to always use symbolic constants without thinking about if it makes sense.

I can also see why it might be good to use a symbol for other reasons. If the context isn’t clear having a symbol here makes it easier to reason about.

I usually end up splitting the difference with something like:

if abs(lat) > 180 / half circle rotation / { }

This keeps context local and meaning clear. It’s always a judgement call.

[-]

Tom_Marien@reddit

Magic numbers most misunderstood rule in code, just saying

[-]

serious-catzor@reddit

[ Removed by Reddit ]

[-]

Extra-Ad5735@reddit

Tell the arrogant dude that tomorrow you'll switch to radians and his magic of not safe Euler degrees will come to bite him in the ass

[-]

hitanthrope@reddit

I think there probably are one or two “magic numbers” you can just leave in. I don’t really need, HOURS_IN_DAY. 24 is fine there I think. Unless there is absolutely no other context.

At perhaps a stretch, if you are working in a space where these values are as “every day” as that maybe you skip the constants, but I’d err on the side of having them.

You’re right and your colleague is a dickhead, but probably not a hill to die on.

[-]

DamePants@reddit

I love how every instantly uses hours in day as an unchanging value when in fact it does change during daylight savings. There’s a reason to use libraries for time calculations

[-]

phatmike595@reddit

There's not 24 hours in a day during daylight savings? Is it 23, 25, or some other number?

[-]

hitanthrope@reddit

Haha, but you know what I mean. Somewhere before you get to ONE = 1 you stop ;)

[-]

dasunt@reddit

I think you mean MIN_INTEGER_INCREMENT. ;)

[-]

DamePants@reddit

lol, yeah, context is the key here, everyone is assuming two things with the 24hrs: 1. We are on planet Earth 2. We are trying estimate days and close enough is good enough.

[-]

ACoderGirl@reddit

For a ton of purposes, those things don't matter. Like, the most common reason for me to specify a literal 24 in my code is stuff like maxDuration := 24 * time.Hour (an hour is the largest duration that Go has a constant for, so it's totally normal to specify durations in days or weeks like this).

[-]

Careful_Ad_9077@reddit

First, I don't think this is your particular case by the follow up but, I will provide anyway.

How long have you been there? There are business domains where the "magic numbers" are so common they don't safisty the anti patterns of being magic numbers. They are as natural for people who have been in that domain as " > 0" would be for most domains.

Funny thing, 0.15 and 1.15used to be like that for finance systems in my country for 30+ years. It was the VAT. Then shit hit the fan when one government decided to increase the VAT to 1.16,

[-]

teucros_telamonid@reddit

Not really relevant, basic math or physics is same across centuries.

[-]

RealLaurenBoebert@reddit

Geometry 2.0 is going to introduce 95 degree right angles in next year's patch, due to inflation

[-]

StorKirken@reddit

Same with HTTP codes in web development, a lot of the time. A lot of devs know the codes better than the underlying names.

[-]

wilderadventures@reddit

Yeah but the bounds of latitude and longitude aren’t changing.

[-]

New_Enthusiasm9053@reddit

In any domain using degrees the magic number is Pi because you use radians not degrees.

[-]

KeytarVillain@reddit

Any domain? No, pretty hard disagree here. Scientific computing, absolutely, you want to use radians. But it sounds like they're dealing with maps here. I have never seen someone use radians for latitude & longitude on a map, ever.

[-]

New_Enthusiasm9053@reddit

Internally you should if you're doing any kind of calculations with either because Pi often drops out so you can do it symbolically with greater precision. Degrees are user facing so you convert to them from radians.

[-]

papa-hare@reddit

Lol that's an amazing example

[-]

echo-whoami@reddit

You were an ass with a completely silly nit comment, and they were an ass with the passive-agressive reply.

No winners here bro.

[-]

antifathrowaway1@reddit

I would have replied "That's okay. As long as I see the value. What I don't see is the cost. You could have done this in less time than it took you to pushback."

[-]

Cykon@reddit

I feel less inclined to make constants for well known, yet whole, mathematical values. For example, if I was working in degrees, I wouldn't make a constant for 90.

I wouldn't think about it too hard here. Disagree and commit.

[-]

Thin_Adeptness_4471@reddit

I bet you'd make a constant for pi tho 😀

[-]

rlbond86@reddit

well known, yet whole, mathematical values

unless you're in Alabama, pi isn't a whole number

[-]

recycled_ideas@reddit

You make (or use) a constant for pi so that you have a consistent rounding, not so people know what it is.

[-]

thefragfest@reddit

If he were my report, he’d get some quick feedback that it’s not okay to be rude to your colleagues. Also that magic numbers should be avoided.

[-]

remain-beige@reddit

Sounds like a dick.

Magic numbers are a widely recognised and well known code smell.

Doubling down on this teachable moment reeks of arrogance.

[-]

IronWombat15@reddit

My general rule is to defer to the reviewer on such easily changed things. He can disagree without being disagreeable though. The comments about chat got and "in case anyone missed this in school" are downright rude, and no one had any business speaking to a coworker like that.

With lat/long especially, it'd be an easy mistake to swap which one was 180 vs 90 capped. Are the constants mostly self-explanatory? Yes. Does extracting them add value and cost nearly nothing? Also yes.

In cases like this, I would forget about the code review nits and focus instead on the disrespectful tone. Bring it up with the coworker first, and consider looping in management if they see nothing wrong with their behavior in hindsight.

[-]

TwillAffirmer@reddit

I am puzzled why the code read "if abs(lat) > 180," which is not possible for latitude, which falls in the range -90 to 90. Perhaps OP is misremembering, or else the codebase put latitude in a range 0 to 180 for some stupid reason bound to cause bugs down the line. If it was really "if abs(lat) > 90," then imo that's clearer than "if abs(lat) > Constants.maxLatitude" because the latter requires me to look up maxLatitude to be certain of what the code is doing. What if maxLatitude isn't 90, but some value representing the maximum latitude of some particular geographic area the program deals with? I'd have to look it up to be sure, but if it just says "abs(lat) > 90" then I already know what it's doing.

[-]

clef75@reddit

Yep. While constants and magic numbers should often be extracted, these aren't really magic numbers: they are part of an algorithm which is inherent to geo location. One of the main reasons you extract magic numbers is that they may change. These won't. East is always 90.

[-]

CodeFoodPixels@reddit

East is always 90, like a constant?

[-]

Remarkable-Coat-9327@reddit

chuckle snorted my coffee into my nose, thanks

[-]

QuitTypical3210@reddit

Binary unsigned rather than signed

[-]

FatefulDonkey@reddit

Well you can use the socratic method. "What happens when the LLM does bulk search and replace and there's magic numbers everywhere?"

[-]

EricThirteen@reddit

“Remove the personal attacks in your comments. This is a professional environment and we like keep our comments clean and uncluttered.”

[-]

MapLarge614@reddit

I have a nice trick in my sleeves. I simply don't accept such commits until they managed to provide a professional commit message.

[-]

Treebro001@reddit

The chatgpt comment would make me question their own competence so much I would not want them on my team anymore and would actively push to remove them.

[-]

sean_lettr_dating@reddit

My strategy in these situations has always been to pull up the old StackOverflow post from old heads discussing the subject of hand, in this case "magic numbers" and the importance of them. It's hard for someone with an ego to defend themselves when a StackOverflow post from 2009 is clearly telling them they're wrong. No longer having StackOverflow discussions to point to is one sad thing about all of this

From Googling "StackOverflow magic numbers" this came up - https://stackoverflow.com/questions/47882/what-are-magic-numbers-and-why-do-some-consider-them-bad

[-]

nephyxx@reddit

If someone pulled this kind of public passive aggressive shit on our reviews they’d be getting a quick chat with their manager. That stuff doesn’t fly in well functioning teams.

[-]

psyyduck@reddit

Yeah, I'm thinking maybe the leadership tolerates toxicity (unfortunately all too common in the US).

[-]

thats_so_bro@reddit

Leadership is usually the toxicity lmao

[-]

unbrokenwreck@reddit

The higher you go the exponential the level. Being a yes man is basically a prerequisite for some designations.

[-]

adgjl12@reddit

What if it’s the manager doing that 🤔 majority of the time he’s a pleasant guy and the backbone of our team but he has those days where instead of taking a break and decompressing he lashes out at the slightest annoyance.

He’d make a similar comment like above but not passive aggressive, just agressive. It’d be something like:

“I don’t see the value and please don’t use ChatGPT to write your comments, thanks.”

This exact situation hasn’t happened obviously but it’d easily be something like that. One time I got really pissed off and told him off and he never responded. We kinda was awkward for a bit but we still work together fine. He did something similar a few weeks ago and this time I really held my tongue and gave a very peaceful/appeasing reply. It’d be something like:

“I didn’t use GPT but I’m sorry if it came off that way. Thanks for getting that in.”

Really upsets me the 1 or 2 times it happens per year but the rest of the times he’s great. The sucky thing is he’s my manager so it’s hard to call out. His manager is worse where he’s a prick maybe once a quarter or so but is otherwise cool.

[-]

midasgoldentouch@reddit

I mean, that sounds like feedback to provide to your skip level manager. Part of being a lead is recognizing that you can’t take your bad day out on your teammates.

[-]

adgjl12@reddit

Skip is similar if not worse. He has his bad days too. Everyone can see the comments including the skip, it’s no secret. Overall environment is good but 1% of the time it can get quite toxic. If I could remove 5 of my worst days out of the year at this job it’s a pretty solid gig.

[-]

CarlCarlton@reddit

My usually-pleasant manager / lead dev of 30 years experience leaving a PR review the instant I forget to create a config for a factory that creates configs for a factory that creates a visitor for a proxy object amidst 72 modified files across 8 libraries just to implement a single feature he budgeted 8 hours for but that actually took 50 to implement: "YOU FORGOT THAT INNOCUOUS VERBAL REMARK I CASUALLY QUIPPED DURING THE DESIGN-REVIEW PRE-REVIEW!! FIX THIS NOW!!!!!"

[-]

CarlCarlton@reddit

My manager / lead dev of 30 years experience leaving a PR review the instant I forget to create a config for a factory that creates configs for a factory that creates visitor for a proxy object: "THAT'S NOT WHAT I ASKED!! FIX THIS NOW!!!!" [translation: "How dare you forget to read my mind!?"] (true story)

[-]

Banquet-Beer@reddit

Do you always go cry to managers over every tiny little thing?

[-]

The_Axolot@reddit

Seriously! When did we become such snowflakes?

[-]

shill_420@reddit

Why do you wanna know?

[-]

nephyxx@reddit

I wouldn’t need to because they’d nip it in the bud before anyone needed to say anything.

[-]

steerpike1971@reddit

Maintainability in case there are 361 degrees in a circle some day?

[-]

soundman32@reddit

We have leap years and leap seconds, maybe we also have leap degrees?

[-]

steerpike1971@reddit

Damn. You are right. They can quickly update the code for when circles get fatter in a leap degree year.

[-]

Normal-Match7581@reddit

Quoting the message That's what a gpt boy would say :)))

[-]

pigeon768@reddit

You need to explain what domain you're coding in. My day job is in GIS. I eat shit like longitude and latitude for breakfast. In some domains you must have named constants for stuff like this, in other domains you must not. If one of my coworkers sent me a PR that had something like #define MAX_LONGITUDE 180 I'd be like bro stop trying to act cool. We all know what 180 degrees longitude means.

That being said, if (abs(lat) > 180) is completely fucking wrong. Min latitude is -90 (the south pole) and max latitude is 90. (the north pole) Longitude gets out to +/- 180 but latitude only gets to +/- 90. You both fucked this up. Which probably, to me, indicates you're not in GIS. In which case you should definitely have named constants for this shit.

Also, even if you did it correctly, like with if (abs(lat) > 90) you're still doing it wrong. NaN will up and fuck your shit up. abs(NaN) > 90 will return false, which might imply that your point is between the north pole and the south pole. Which NaN fucking isn't. The right thing to do is something like if (-90 <= lat && lat <= 90) or whatever.

[-]

Secret-Wonder8106@reddit

Are you unable to have a cohesive thought without sprinkling "fuck" and "shit"? relax

[-]

rooster_butt@reddit

There is a large chance OP is misrembering his coworkers code because he doesn't know the difference between lat and long.

I also work on GNSS receivers. Also not allowed magic numbers except of trivial things. Lat/long falls under trivial.

[-]

official_business@reddit

I'd leave the NAK on the review put him on ignore mode for a few days after those comments.

If the manager comes along and asks why I wasn't approving the review, I'd just show him the review and let the manager decide what to do.

Having said that, I've been the mouthy little shit on the other side of the fence. I once did

 #DEFINE FIVE 5

and sent the review back for a bit of a giggle.

[-]

soundman32@reddit

I've seen this in a large POS code base.

define ONE 0

[-]

official_business@reddit

It might be OK if you wanted a very small value of one.

[-]

iso20715@reddit

I wouldn't call 90 degrees or 180 degrees magic numbers

[-]

soundman32@reddit

If you have to imply what a number means its a code smell.

Luckily this variable is called lat, which is an abbreviation for latch which means on or off, so 180 is the wrong number. Hmm.

[-]

soft_white_yosemite@reddit

These days, I'd give someone a written warning for a comment like that. I'm so over wankers.

[-]

skidmark_zuckerberg@reddit

Legit don’t know why some people act this way. I’ve disagreed with PR comments many times but I’ve never been an ass about it or took offense. Software development is a team sport, and you can’t respect someone who’s a dick.

[-]

GoodishCoder@reddit

Some people can't handle criticism at all and take it super personally, even when it's just about the code

[-]

OrangeBagOffNuts@reddit

Refused the pr, point the readability issues - see who dies first in that hill

[-]

TehBens@reddit

Doesn't latitude only go from -90 to 90? That's at least what I found via google.

devs should already know this.

Does the whole team work in a domain where such constants are used regularily? In that case, he might be correct. But even then, or in particular in that case, you should have a defined list with such constants, because otherwise somebody will use incorrect values for some constants eventually.

[-]

ajamdonut@reddit

Do I blame the dev cracking on, or the guy so defensive he writes a reddit post. I already know the reality. One's doing work, the other is on reddit. So what if you're right lol, do proper work.

[-]

SLW_STDY_SQZ@reddit

It depends on the context. If your product is specifically a geospatial oriented one, or your company specializes in this type of product, then I'd say it's not necessary. But if this is just a small feature in an app that otherwise has nothing to do with geo then you are correct to suggest a constant imo.

[-]

forbiddenknowledg3@reddit

At least you guys care about this. Now that AI writes all the code nobody in my team gives a fuck.

[-]

ananiasanom@reddit

Your suggested change makes the code less understandable. They are right and you are wrong. You should be working for them.

[-]

CompetitionNo3466@reddit

Get a manager in for this and meeting with the 3 of you

[-]

Ok-Rule8061@reddit

I think your feedback was wrong - 90 and 180 really aren’t magic numbers when it comes to angles and their usage as literals should be self evident and actually enhance readability and rapid understanding rather than hiding them away in constants.

I also think his reaction was wrong, and you both missed the opportunity for a lively and respectful debate on the matter that could act as a learning opportunity for both participants, and anyone else coming along to read the review later.

Poor show by all, I’m glad I’m not on this team!

[-]

poweredbyearlgray@reddit

Kinda agree with both. You’re missing a style guide as others have said, but I think maxLatitude is a pants constant name. I’d have suggested AngleConstants.degrees180 or something. That seems more reusable in other contexts.

Best of both worlds, units clear (so you don’t get mixed up with Radians in future), you get the intuitive number inline too.

I’d maybe then annotate with a comment that 180 degrees is max latitude.

Snarky dev is at least comfortable to give frank feedback so would try to shape that into a learning experience rather than authority-squash them. Make them write the new standard, maybe set them a task to fix all other non compliant constants in the code base and/or set up a linting rule if that fits your stack.

[-]

MaestroLifts@reddit

"Plus latitude and longitude is something we learn in school"

You also learn not to use magic numbers and how to write self-documenting code as a freshman in college, but clearly no everybody learns.

[-]

glowingGrey@reddit

Honestly, from what you’ve written here I wouldn’t want to work with either of you.

On the magic number; they might have a point. Factoring out well known numbers in a domain may lead to obfuscation of the intent behind it, and this *clearly* is some kind of angular calculation where numbers like 180 will be well known and have meaning. It depends a bit on the code and what’s around it, but it’s not a given that changing the number to a constant helps.

Their attitude is poor. But your PR suggestion is also poor, the constant name `maxLatitude` is a classic, no-effort constant name that adds nothing in terms of code self description. *Of course* it’s a maximum latitude for that line of code, there’s a conditional check to see if `lat` goes above it. But is that maximum for the latitude everywhere in the code? What does the maximum mean, is it just a wraparound, an error or some other logic? If you’d actually written a thoughtful review which suggested a name that encoded some decision logic then you’d likely have got a less snarky answer than making it look like you took a generic recommendation from ChatGPT.

[-]

bothunter@reddit

We all know the value of pi, but I'll still fail a code review if I see 3.141 sprinkled throughout the code.

[-]

03263@reddit

Every language I've worked with has a predefined constant for pi

[-]

g0fry@reddit

Nobody knows the value of Pi 🤷‍♂️

[-]

jrodbtllr138@reddit

Any real engineer knows Pi=3

[-]

bothunter@reddit

Unless you're in Indiana, then it's 3.2

[-]

rodw@reddit

To be pedantic if you're gonna cut it off there it should be 3.142 anyway

[-]

bothunter@reddit

Which is exactly why it should be defined as a constant!

[-]

No-Mention-9653@reddit

Hey , I know it's hard to take suggestions from colleagues, but there is no need to be unprofessional about it. Adding no magic numbers is a very basic concept that improves readability in the future. We all might know now but new Devs might struggle with what's easy for you. I hope we can have more professional discussions in the future without this childish behavior.

[-]

g0fry@reddit

Ah yes, passive aggressive texts work really well in a professional environment, people just love it 👍🤦‍♂️

[-]

No-Mention-9653@reddit

Everything for escalation

[-]

nydstyrk@reddit

I worked with a person like this. They were introducing a super toxic atmosphere into the team. Fortunately I wasn’t the only one thinking that as the person was fired after a few months with no improvement in behaviour.

[-]

Dziadzios@reddit

Job security driven development.

[-]

tiajuanat@reddit

Even if you're running latitude, it is insane to me that every developer is expected to memorize all the intricacies of every domain they ever work in.

Like imagine needing to memorize that 0.3575 is the K2 coefficient of pure aluminum, and a mol is 6.02x10^23, and tomato is botanically a fruit, but culinarily a vegetable. No other field demands that level of absolute pedantry from all its members all the time. Yet, we have folks like your colleague that think it's normal.

[-]

Fit-Knowledge-3714@reddit

Basically they place low value (or have no understanding of) code maintenance. And to argue the point underlines they are an idiot

[-]

WesolyKubeczek@reddit

Ah yes, the ever present danger of maximum latitude or longitude ever changing.

[-]

IEnumerable661@reddit

I can kind of see the half way line here.

For me, magic numbers are more along the lines of, if its good, return 1; if its bad because reason X, return -1; if its bad because reason Y, return -2.

Or, if authLevel is superuser, return 5; if authLevel is standardUser, return 4, etc.

Those magic numbers are far more awful to have to deal with and absolutely need enumeration or some sort of global constant.

And yes they are littered all over our current codebase in around 40 different solutions all of which have to agree what those magic numbers are and what's stored in the database in several different places. Unwinding all of that is a job nobody appears to want for some reason as getting it wrong is tantamount to seriously huge explosions. Hence, its been left.

[-]

srdjanrosic@reddit

I hate reading code where every actual value is a gazillion page ups away

I prefer if abs(lat) > 180 { ... } // 180 degrees, max latitude

btw, you sure its latitude not longitude?

Unless we're talking about Pi, or e, or similar, then by all means use whatever is in the math library, it's there for a reason.

But the code that does

SECONDS_PER_MINUTE = 60
MINUTES_PER_HOUR = 60
HOURS_PER_DAY = 24
SECONDS_PER_HOUR = MINUTES_PER_HOUR * SECONDS_PER_MINUTE


..


elapsed_time / SECONDS_PER_HOUR

like dude,... just do 3600, .. or if the language has built-in use that, or if you use 86400 or 43200, 900, put 24h / or 12h / 15m in a comment on that line. Don't make me read you compute seconds per hour for the umpteenth time.

.. and if you need to display duration, use whatever "humanize" module or library people use, don't invent your own logic for it.

... anyway ...

[-]

Professional_Hair550@reddit

I do 246060 instead

[-]

PerryTheH@reddit

I think this post is less about the nitpick and more about the passive aggressive comment of the other dev.

[-]

coderemover@reddit

This ^ guy knows how to write good code.

Adding to that - there is not even point in using values like 86400. Just leave 24*3600, compilers are smart enough to optimize it and it’s much more obvious to the reader where the constant came from.

[-]

PriceSpiritual8223@reddit

Why dont you just fight him

[-]

odd_commenter@reddit

Didn't we learn not to use magic values in school too? Sloppy coding not to.

[-]

carayThree@reddit

If those numbers are duplicated, it isn't DRY. If the meaning of the duplicated numbers is different, you'll be in for a world of pain.

[-]

Level-Courage6773@reddit

Have I understood this right, he used ChatGPT to make the changes you recmmended?

If so, I'd wager that he uses ChatGPT for a lot of his 'work' and hardly understands any of it, and so used his arrogant responses to deflect from showing how bad he is at his job.

[-]

PerryTheH@reddit

As a team lead if I saw any member of the team answering this way to others I'd suggest a PiP for behavior.

Some devs are amazing and they bring a lot ti the team in terms of knowledge, but I rather have a normal dev that knows how to behave like a decent human being than this pos person.

If this is it's first time I'd probably chat with him/her, maybe a bad day or whatever, but if this is a repeat offender I'd look to get rid of it. I used to work with a dev that was passive aggressive with everyone on the team and nobody wanted to work with him, it was a horrible work environment and people would stress out about him. He even made a Jr cry out of frustration.

Anyhow, I hope this pos person is reported and handled appropriately.

[-]

Popular-Jury7272@reddit

I wouldn't have been such a dick about it but I would have argued that 90 and 180 are NOT magic numbers in this context. Anyone qualified to be working on that system would absolutely know what they represent.

[-]

Careless_Dingo_7793@reddit

Those comments are abuseive nothing less, if you accept them and do nothing then you don't value your own self worth.

They are in writing and can be associated with who wrote them.

If it were me I would call it out on whatever message platform your company uses.

'Hi #name# I saw your code review comments and wanted to let you know that your comments are abusive, please do not leave comments like this again', Then I would message there direct line manager and state 'Hi Name I received abusive code review messages from #Name#, I have asked #Name# to not do it again but wanted to make you aware as this is unacceptable behaviour in the workplace. Then I would Email HR and state 'Hi HR, I received abusive code review comments from #Name#, I have made #Name# and Name aware and ask for them to ensure this does not happen again, if you could also make note of this and prevent reocurance it would be appreciated'.

May seam a bit much but those that abuse others need a kick in the ass, this guy is likely doing it to others and probably worse outside work. A female friend of mine got abusive messages for a year from some guy who got her phone number from a Gumtree add, all because she looked pretty in her profile picture. Kept finding ways round the blocked number, caused her to doom spiral, played on her insecurities, took me ages to figure out what was the problem as she wouldn't talk about it and eventually started talking to him and the chats were abusive and disgusting. Luckily I managed to get through to her before anything bad happened, others may not be so lucky.

So Report it, hope it kicks him in the ass, and you may just help someone you never know.

[-]

cambridge-resident@reddit

Why was the comment not that using lat as the variable name for an angle that is not constrained to the range -90 degrees to +90 degrees is confusing. Compounding the error by naming the value maxLatitude is even worse as anyone knowledgeable will probably assume that constant is 90 degrees.

You would be correctly limiting the range if you were working in half-degrees, but I doubt that is the case.

[-]

PlasmaFarmer@reddit

You were completely fine. The other dev acted like a kiddo.

[-]

kreiger@reddit

and similar checks using 90 and 180 regarding coordinates.

I asked what those numbers meant

How do you not know that 90 degrees is a quarter circle, and 180 degrees is a half circle?

Those are not magic numbers, they are fundamental to math.

That's like saying x * DOUBLE instead of x * 2.

In general you would be in the right, but in this specific case the numbers are easier and more natural to read.

[-]

KTAXY@reddit

Put your foot down and stand your ground. This hill is one you defend until there is nothing left.

[-]

ff3ale@reddit

So OP's codebase just has one Constants class where all the numbers live? And that's better somehow than having the number close to the calculation?

[-]

kruperfone@reddit

Personally I think that setting variables for numbers all the time is too much and makes code less readable, but adding a comment in this case would be nice.

But this part with chatgpt reply, that's off limits

[-]

Spare-Builder-355@reddit

hahaha I don't know if it's typo in the post or exact code, but in case it's exact code, hear me out.

Max latitude is f***ng 90 degrees, AS YOU SHOULD KNOW FROM SCHOOL, you smart-ass-PR-author.

Using properly name constants would absolutely avoid this stupidity and also future stupidities when someone else will be sure they "just know the correct number".

[-]

FatefulDonkey@reddit

Your intuition was right, but you tried to over correct.

Instead of expecting a specific structure "enums" let the dev figure it out themselves. Unless it's a policy in this codebase to use only enums. You should just tell him to prefer a constant variable.

Personally I would have prefers a global all-cap for readability. MAX_LATITUDE

[-]

angryapathetic@reddit

"This has nothing to do with latitude and longitude and whether you learnt it in school. It is about best practice in managing magic numbers. Thank you for making the change"

[-]

iamdecal@reddit

> “Nice usage of ChatGPT :)) I still don’t see the value.

what they're getting back from me is

"Hmmm... okay, i will see if we can get a training budget allocation, so you can go over the basics"

[-]

coderemover@reddit

They were right. If you don’t know the range for latitude and longitude, then extracting the bounds to separate constants is likely not going to improve your understanding of the code, because it seems you’re missing some fundamental knowledge about coordinate systems.

[-]

martinbean@reddit

Blindly applying rules like „no magic constants” quickly ends up in nonsense like:

const ZERO: u32 = 0

No, you don’t. What a silly argument.

You use constants in place of “magic” numbers to give meaning and context; not just for the sake of replacing an integer or float with a constant.

[-]

ClayDenton@reddit

A linter would have picked those up as magic numbers, it's a well known rule so it's a reasonable review comment. There are contexts where magic numbers make sense, sure, but pushbacks should be professional.

Personally I'd see this as an attitude problem and either give the dev direct feedback that they have been unprofessional or let their line manager know the same.

[-]

no-sleep-only-code@reddit

If a couple constants is “unnecessary complexity” maybe they can’t handle dev work lol.

[-]

YahenP@reddit

Sometimes. I emphasize , sometimes, acceptable to use a magic number. If, for example, it's used once, and its origin is clear from the context, and the reviewer agrees. This can reduce the cognitive load. But it's never acceptable to leave such comments in the code. And in any case, moving the magic number to a constant is not a matter for discussion. It's a request that the code author must fulfill.

[-]

spindoctor13@reddit

I completely agree with them not you on the magic numbers, I think extracting them as consts harms readability and maintainability, but it wasn't a very mature response by them

[-]

CodelinesNL@reddit

At a high level, I think this one is pretty minor. Is Math.PI easier to read than 3.14? Not really. Is it nice to at least be consistent with "magic numbers"? Absolutely. There are really no good reasons to not follow your approach, even in simple situations like these.

The passive-aggressiveness of the comment is however a much bigger issue. I would have an in-person one on one chat with them about it, that I value an environment where we can discuss things professionally. And that while in this case it does not matter that much for readability, not having magic numbers in code is a pretty standard practice and that even in this case, it would be nice to stay consistent.

[-]

randomInterest92@reddit

"devs should just know" is actually an argument for not using the number. Knowing stuff is inefficient.

[-]

MangoTamer@reddit

Are you ****ing kidding me? Who uses variables for increments of 90 degrees when working with radian or degree math?

YTA, op.

[-]

horse_examiner@reddit

I've always worked at tiny like 15 people companies so snarky comments like this on PRs has been acceptable, people even swear on them, but that's a very small company there are no managers who are going to feel like they're a little bitch of they don't stand up for their dev or not, so I think it really depends on the company size and who sees this

But aside from cultural stuff honestly I find it kind of annoying when people want a define for things like 90 degrees, it's like bruh that almost makes it more likely to have a bug just because the definition isn't near the use so if it were changed less likely to be caught

[-]

brstra@reddit

He’s a toxic dick. I would definitely let know my manager.

[-]

drachs1978@reddit

The purpose of code reviews is to make sure the code does what it's supposed to do. I.E. to prevent insider threats, bugs, and other security problems. If your company has a published style guide, it's also fair to stand fairly hard on it.

However your personal nitpicks are just that. The other developers on the team have no responsibility to make style changes just because you prefer a different style unless they report to you. If the issue isn't a bug or a security problem just move on with your life.

Complain to your manager if you feel like your peer is creating a hostile work environment but do not engage in petty back and fourths in the PR history - That's only going to make you look bad. Do not let other people's attitude problems put you in a position to reflect poorly.

If you're giving him good advice and he's blowing it off, that's a him problem. If you feel like something really is a serious bug or security issue, block the PR.

[-]

BothWaysItGoes@reddit

What is "Constants.maxLatitude"? I would be more confused trying to understand what it means compared to 90. Max latitude of an object? Max latitude of a projection? Something else? He is correct that it adds no value.

[-]

Maleficent_Tank2199@reddit

In this context those values are not magic numbers. There are no external reasons for these to change neither are they arbitrary fixed numbers.

Depending on code context a named constant could make sense but this depends on the actual code.

I think you where incorrect to press on that matter.

Now his response however was immature and unprofessional. This needs to be discussed and addressed.

What is your teams conflict resolution strategy? Talk with a mediator / TL?

[-]

RickAmes@reddit

i get where you are coming from but if you're working with lat/long then it's better you understand how they work.

depending on the context i would rather force the dev to look it up.

for example where is 0,0?

i had a dev try to use them as defaults for the lat long values before.

[-]

lepapulematoleguau@reddit

Bad attitude from that dev.

Oh the other hand I don't think that extrating to a Constants enum is that great of an idea. That's how you quickly get a Constants enum full of unrelated values.

I think it's a good practice to ensure Constants enums are categorized, having multiple is needed.

Even better, using a local constant or extract to a well named local method or function.

[-]

MCFRESH01@reddit

Bring it up with your manager in a 1 on 1. You are right and the snarky chat gpt comment has no place in a code review. Sounds like he’s all ego and probably inexperienced and needs to be brought down a peg

[-]

polaroid_kidd@reddit

No, you're not. We have a fairly strict "no magic numbers" rule, with the exception on -1, 0, 1 and... 2 I think, exclusively for array access.

Everything else is a constant. Some of them are just defined in the same file and used once.

I don't have enough time to play the guessing game about random numbers. The dev needs a reality check by the lead. Snarky comments like this can and will have an adverse effect on team dynamics.

Lastly, IDGAF, if Claude, ChatGPT or the Ouija Board came with with a comment on a PR. If a dev thought if was good enough to pass on and defend then it's should be assumed as the Dev's comment.

Lastly^2, for the love of god, align the team on the use of magic numbers. This is such a no brainer. Either you allow them at your peril or you get rid of them. Ether way, repeated discussion s like this on PRs is a waste of everyone's (limited) resources.

[-]

Icy_Accident2769@reddit

We have the rule: you can use all the AI tools approved by security. Anything that comes out and is actually used is deemed to be written by you and you will be judged that way.

AI is a tool just like your IDE, you can’t use the argument “this is what ChatGPT said”

[-]

polaroid_kidd@reddit

Samsies. It's been a good send reel have everyone agree on that.

[-]

SemaphoreBingo@reddit

No, you're not. We have a fairly strict "no magic numbers" rule, with the exception on -1, 0, 1 and... 2 I think, exclusively for array access.

You must not write a lot of math code.

[-]

polaroid_kidd@reddit

You're absolutely right. It's a banking frontend. I'm be damned if I let the PM start forcing us to calculate finance related numbers in JavaScript.

[-]

g0fry@reddit

Ah, so your use case is valid, but their use case is not. 😄 You’re special, they are not.

[-]

Kazumz@reddit

I’d be booking a catchup style meeting in with them for a few days later after I’ve calmed down and can approach it professionally.

You just let them know your thoughts, and the facts, and keep the feelings out of it.

At the end if there’s no resolution just let them know you’ll be looping in the SEM to take the subject forward as next steps.

Sometimes people forget they’re in a workplace, other times you need to stand up for yourself.

[-]

VizualAbstract4@reddit

For me, no room for debate. No question. “Magic number. Move to constants and clear name variable”

“Devs should know”

Will earn them a reply, “As should you. No magic strings or numbers.”

[-]

g0fry@reddit

So if you are creating a time duration variable, you do:

const int HOURS_IN_A_DAY = 24;

const int MINUTES_IN_AN_HOUR = 60;

const int SECONDS_IN_A_MINUTE = 60;

const int MILLISECONDS_IN_A_SECOND = 1000;

const int THIRTY = 30;

var thirtyDaysInMs = THIRTY * HOURS_IN_A_DAY * MINUTES_IN_AN_HOUR * SECONDS_IN_A_MINUTE * MILLISECONDS_IN_A_SECOND;

instead of simply writing

var thirtyDaysInMs = 30 * 24 * 60 * 60 * 1000;

?

[-]

VizualAbstract4@reddit

What are you using it for?

Don’t be so redundant or salty. It’s like people love making up problems to justify their logic.

const PAYMENT_JOB_EXPIRATION = 24 * 60 * 60 * 1000 * 3:

[-]

g0fry@reddit

You didn’t read my comment, did you?

[-]

darkslide3000@reddit

The point of creating a constant in a case like this is avoiding typos, not so much explaining the purpose. Plenty of code bases using 1204 bytes for a kilobyte out there. But I think it's arguable whether maxLatitude is really clearer than the degree number everyone is familiar with, so maybe consider just naming the constant k180deg or something like that.

[-]

ghost_jamm@reddit

It’s also just easier to change if it’s defined in one place. Granted, lat/lng stuff probably won’t change much but you could decide to use Mercator bounds of 85.5 instead of 90 or something and then it’s a one line change.

[-]

Montaire@reddit

The limit of 180 isn't going to change. Thats like saying "if we get more than 60 seconds in a minute"

[-]

seven_seacat@reddit

leap seconds have entered the chat

[-]

g0fry@reddit

Well, there are leap seconds. So some minutes have 61 seconds and some have 59. Very few, though. And for normal calculations they are irrelevant.

[-]

ghost_jamm@reddit

Or you could have wrapped coordinates like 270 or 360

[-]

SemaphoreBingo@reddit

Mercator bounds of 85.5

Anybody doing this deserves whatever bugs they get.

[-]

ghost_jamm@reddit

I have worked in geospatial software for a decade. There are plenty of legitimate reasons to do this.

[-]

Future-Duck@reddit

Or someone fat fingering a magic number in one place and it’s 189 in one place but 180 in all others. Sure a code review would hopefully pick that up, but things like that do slip through.

[-]

seven_seacat@reddit

As someone who constantly mixes up latitude and longitude, I would appreciate some kind of signal to know that these are the bounds, not just some arbitrary number we're validating aginst

[-]

Secret-Wonder8106@reddit

He is correct. Those are simple mathematical premitives when working with angles, making a variable for 90 and 180 is an overkill.

[-]

allknowinguser@reddit

That’s someone just being ignorant. Magic numbers are a pretty basic rule. The way they handled that is just stupid.

[-]

TheMightyTywin@reddit

90 and 180 in this context are not magic numbers

[-]

divinecomedian3@reddit

But they're constants so just make them constants

[-]

Sir_Edmund_Bumblebee@reddit

The main reason to pull out constants is to be able to change them easily though. If it’s an inherent number then what’s the reasoning for making it a constant?

Like the number of milliseconds in a second is also a constant, but pulling that out as a constant seems needlessly roundabout.

[-]

SansSariph@reddit

I'm not sure it's true that's the main reason. It's a great reason. There are multiple high value reasons.

Semantic clarity and mutual understanding is another reason. Whether extracting a constant aids clarity or obfuscates it is part of the art or craft here and is the topic of this very thread 😀

Milliseconds to seconds is low cognitive load - it's SI, base 10, easy. A constant for the ratio of kilometers to miles is also a "constant in the world" but is a great case for an extracted constant because it has value exactly in being a constant, shareable, reusable, documented thing in one place.

[-]

g0fry@reddit

Ratio for kilometers to miles is definitely not a constant, because a mile is not a constant (there are various definitions of miles). Also, that ratio is not a whole number, so precision is a factor too. And you want to use the same precision everywhere.

[-]

Sir_Edmund_Bumblebee@reddit

Sure, but at least personally I’d consider 90/180/360 degrees to be pretty obvious and low cognitive load in this sort of context, no? I’d consider it to be more akin to the number of milliseconds in a second than the conversion of km to miles.

[-]

diablo1128@reddit

At places I have worked you do it because there is a coding standard rule that says no magic numbers, expect for . Time has not been an expectation, so you just shrug and do it. Chances are it has been defined somewhere already and you just use it.

Not a great answer, but it's easier then arguing with people to chance the coding standard.

[-]

Sir_Edmund_Bumblebee@reddit

Interesting, I haven’t worked anywhere so dogmatic with coding standards. At least in my opinion there are very few standards that are correct 100% of the time and it’s important to allow for good judgement.

[-]

SemaphoreBingo@reddit

It's (probably) not fortran, they're not going to be re-defining integers.

[-]

xeric@reddit

It seems like it would be very easy for a dev to mistakenly mix up the maximum lat and long values in a later refactor, and no one to notice the regression.

Whereas if you had constants for Longitude.Max and Latitude.Max it would be very obvious if you’re using the wrong value.

[-]

ub3rh4x0rz@reddit

Without siding with either party, it is not a magic number. A magic number is something like "5 means New York". This is seemingly just a number.

[-]

laccro@reddit

Magic numbers are usually just any number that appears in code without an explanation https://en.wikipedia.org/wiki/Magic_number_(programming)

[-]

ub3rh4x0rz@reddit

The problem with that link is that it confirms what I said, not what you said.

[-]

beary_potter_@reddit

Does it? Example they give is sales tax.

[-]

ub3rh4x0rz@reddit

The example as written happens to be a bad example in that it is overly contrived. The summary on the page does a better job, first in semantically defining it as a number having special, particular meaning, and second by offering real examples: numbers representing file formats and guids.

[-]

SubstantialEqual8178@reddit

The coworker OP complains about was being rude, but was not wrong that anyone understanding latitude - which is fair to assume - would know the equator is 0 degrees, and anyone knowing basic trig would know that you can only go 90 degrees above or below considering 360 degrees is a full rotation.

And yet the code snippet being discussed is abs(lat) < 180, suggesting that either:
- This team isn't actually that familiar with GPS coordinates
- Their codebase is representing latitute in a non-standard way (0 to 180, instead of -90 to 90, for example)
- They've mixed up latitude and longitude

In any of these cases, a constant would be useful: the first because it documents what the values mean to a team for whom it's not obvious; the second because it allows the programmer to use the abstracted value and not have to worry about the odd way it's implemented; and the third because abs(lat) < Constants.MaxLongitude

[-]

IShitMyselfNow@reddit

first in semantically defining it as a number having special, particular meaning

but was not wrong that anyone understanding latitude - which is fair to assume - would know the equator is 0 degrees, and anyone knowing basic trig would know that you can only go 90 degrees above or below considering 360 degrees is a full rotation.

Pick a lane and stick in it.

[-]

ub3rh4x0rz@reddit

Do we need a constant for 0 then? It has "special, particular meaning" according to you, I guess.

[-]

IShitMyselfNow@reddit

I think the deck size is a good example.

You should know that a pack of cards is 52 cards. It is perfectly readable if the number 52 was used instead. But extracting out 52 to a constant is better IMO.

[-]

hippydipster@reddit

What if Pinochle is my game? Or Euchre?

[-]

ub3rh4x0rz@reddit

A deck of cards is an arbitrary construction, beyond any attempt to categorize axiomatic mathematic systems like trigonometry as arbitrary. Precomputing 52 + 1 is arguably the bigger problem in that contrived example than not using a deckSize constant. Still, it is not a magic number, it is just not super clear code. These contrived examples undermine the purpose of the page which is to explain a concept, as evidenced by these repeated misunderstandings. The "magic" part is that there is essentially an implicit lookup table that is not referenced in the code, and the real examples given in the summary fit that statement.

[-]

An4rk-yy@reddit

Nit...would not pass the same code review the post is talking about.

[-]

tugs_cub@reddit

There’s a perfectly valid argument either way for something like this. To reuse an example from another comment, you wouldn’t feel the need to make a constant for HOURS_IN_DAY (unless you were one of the more annoying, fake-productive people I ever worked with). This is not quite as obvious, but the value is still:

short
unchanging
general knowledge if you’re working in the domain

It’s on the level where I wouldn’t pick a fight if somebody asked me to make a constant for it, but I also probably wouldn’t ask somebody to make a constant for it if I were the reviewer.

[-]

DamePants@reddit

Hours in a day are not constant, and it does matter for the calculations I have dealt with.

[-]

tugs_cub@reddit

Depends on which definition of “day” you’re concerned with, but yes sometimes that is also true.

[-]

g0fry@reddit

In my opinion, you are the asshole. Because given a context, some numbers already are enums/constants, not magic numbers. You just don’t understand the context and you are just blindly applying the “no magic numbers” rule.

Constants.maxLatitude is a horrible name for a constant! Is it max overall? Is it max that the plane/boat is allowed to fly/sail? Your name of the constant makes it LESS readable.

And then when the dev rightfully said there’s no value in it (he should actually have said that there’s negative value in it), you were being condescending and just repeated what every dev already knows, as if you were talking to a kid.

Actual magic number would be something like:

int rotationDeg = 65;

That’s a proper magic number, because nobody knows where the hell those 65° came from and why is it 65.

But if you write

If (angleDeg > 90) then { … }

everybody will understand that something is happening when the angle is more than a right angle. And if they don’t, they have no business reading the code in the first place.

[-]

SwillStroganoff@reddit

So some formulas use multiples of pi

[-]

MoltoAllegro@reddit

I also could use the literal 3.14195, and yet Math.Pi exists. I wouldn't presume anyone knows why this is 180 - I wouldn't off hand.

You could make the argument that lattitude/longitude should be their own types with validation in the constructors/mutators. There are probably libraries for geographical data and calculations.

[-]

g0fry@reddit

Depending on what you do, you may need various precisions of Pi and then you want a constant. But 180 is always 180. It will never be sometimes 180.34, other times 180.3489 etc.

[-]

authentic_developer@reddit

The framing of "magic number vs. not" misses the actual question, which is whether a name would add information. MAX_LONGITUDE = 180 tells you nothing the number doesn't already tell you in a geo context. VALID_COORDINATE_RANGE_DEGREES = 180 starts to actually document an assumption. If the name you'd give it is just restating the value, leave the number. If the name captures a business rule or a non-obvious constraint, extract it. The snarky response from your colleague was unnecessary regardless of who was technically right.

[-]

Zulban@reddit

Sounds like you're both in the wrong.

If the code says "(function_about_angles) (operator) (divisible by 90)" then I think insisting on replacing the magic numbers is overkill. If that's the worst thing about the MR/PR then your team must be brilliant.
They committed an unprofessional childish comment.

I always try to categorize my CR feedback as either:

nitpick: feel free to ignore, I just have to say it
suggestion: you must reply, I'm open to us not doing this
requirement: this must be done to accept the merge

I precede almost all my CR comments with "nitpick:" or "suggestion:" etc.

Your magic number thing here is a "nitpick". You're wrong to insist.

However sometimes magic number fixes is a requirement.

[-]

papa-hare@reddit

I do nits, but I don't do suggestion/requirement, that's a great approach actually!

[-]

ACoderGirl@reddit

I do the same for most of my comments. Honestly, probably most of my comments are prefixed with "Nit:". When I feel something is extra nitty, I often do something silly to make things more casual, like "NittyMcNittyface:" or "Super nit:" (usually for style guide stuff where it's more about consistency than correctness).

I don't usually preface my "regular" severity comments. If I leave more than a few comments, I might prefix super important stuff with something like "Important:" (such as for stuff that will cause a devastating outage if missed).

I also often prefix questions with something to make it clearer that they aren't necessarily blocking but more of a "please consider this carefully" (classic example being "under what circumstances can we even hit this?").

[-]

Zulban@reddit

NittyMcNittyface

Indeed. I also put a lot of thought into being as silly as possible but always serious when it matters.

[-]

saposapot@reddit

That's a great suggestion I would do moving forward.

[-]

Tango1777@reddit

Yea you don't work with very good devs, you work with devs who think they are very good themselves. Which usually means they are completely average. Self-explanatory code is coding 101, any hardcoded values are bad and it's no excuse that something is common knowledge. Especially that common knowledge is vague and also changes from one culture to another and from one business to another. Hardcoding is bad in any form. And "devs should know" is a very bad excuse, because reality is they do NOT. Take a code even you wrote yourself, wait half a year, go back to it and you'll realize you forgot a lot about it unless it's clean and self-explanatory, which means it easy to pick up again. In that case you are right and either you fight for it and make them improve or you accept the fact you're among devs who don't really care about proper standards. But if you let them, it'll get worse in a long run, so beware. Also usage of AI is normal these days and is not a reason to feel ashamed. Especially that it's so easy to prompt AI now to properly do such refactors improving code quality and cleanliness. Feel sorry for you, tbh.

Nit picking is not a problem anymore, you do it a few times, even very minor stuff, every time you just add an instruction to your AI guidance file and one feature after another AI improves its work and those easy to avoid mistakes no longer happen, because AI has it clearly instructed to follow a certain way. Where is here a problem here lol.

[-]

recycled_ideas@reddit

If the meaning of a value is well understood using a constant just to avoid magic numbers is actually bad design because they really do add unnecessary complexity.

There's a few questions you can ask in these contexts.

Is the meaning of the value clear to someone who understands the domain of the code clear? In your example if you don't know or can't quickly understand the max values of lat and long you shouldn't be touching code that uses it.
Is the value likely to change (everyone in your company might understand what the sales tax is, but it might change.
If it's likely to change is it used in multiple places. If you only ever use calculate sales tax in one place and it's clear that it's sales tax creating a constant for "sale tax" constant is pointless.
Is there a possibility of differential rounding. Pi is a good example here. Even though pi doesn't change and we'll know pi when we see it, if I round to three digits and my colleague rounds to 10 we can have different results.
Can I come up with a name for the thing that adds value. Max latitude is a name that makes sense, kinetic energy constant for the 0.5 in the kinetic energy formula is not.

Your colleague's response is snarky, but if I'm brutally honest, junior and intermediate level developers who treat development guidelines like commandments from God kind of piss me off too and that's what you've done here.

[-]

doryappleseed@reddit

Ask them what happens if they need lat and long in a different coordinate systems rather than the standard lat/long (like Northing and Easting etc, decimal degrees vs arcminutes and arcseconds), or what would happen if they got/needed coordinates in Albers. A two line change suddenly becomes a day long expedition to find all these references.

[-]

alerusey@reddit

I would ask in the slack channel for all the other devs of the team what they think about it.

[-]

Gabe_Isko@reddit

I think you were being pretty reasonable. Hardcoding values inline like that can really bite you. Had a system go down the other week because a date evaluated against a timeout was hardcoded.

[-]

Telos06@reddit

I'd say they are partially right. Explaining magic constants is especially important when the values are subjective or otherwise non-obvious, and there could be room to debate changing them in a future release. Max latitude won't ever change, so having an extra layer of indirection is slightly cumbersome, especially if you're very comfortable in the domain.

Where I think they are wrong is that:

Not all readers will know off hand that latitude has a range of 180, longitude has a range of 360, and both can go negative. Documenting for such readers is nice to do, and not all that hard.
They sound rather snarky about it. Not a good attitude for team cohesion.

So I can see where that are coming from, but overall, I'd agree with your feedback.

[-]

Imaginary_Maybe_1687@reddit

That being said. If you dont know the range of latitude and longitude, I'm not sure you should be touching that code /hj

[-]

g0fry@reddit

I’m 100% sure they shouldn’t even be looking at the code, they shouldn’t even be nowhere near the code.

[-]

chris-top@reddit

Re-review, remove the no value comment. I am like you, used to take offense in others’ ignorance. You can’t save them all.

[-]

coderstephen@reddit

Passive aggression level is now at DEFCON 3. This is not a drill. All forces should be on full alert.

[-]

KingMaster1625@reddit

Both of you are right. You have a point about readability and maintainability, he has a point about learning it in school. I’ve seen this quite often with developers who are smarter than average. They get annoyed when they have to constantly explain to other people what they think should be a common knowledge. Oh well…

[-]

_some_asshole@reddit

Just add it to the linter

[-]

aaronfranke@reddit

You're wrong, they are right. Those are not useful constants. For angles like 180 degrees, they should be hard-coded. Redirecting to a constant adds overhead for reading that code in the future.

[-]

britnastyboy@reddit

Off with their head! If it’s so easy to do, just do it. It doesn’t add huge value, but it doesn’t not add value.

[-]

dmcnaughton1@reddit

12 YoE engineer and 2 YoE Engineer Manager, you're correct in this situation. Magic numbers aren't a good habit to get into, and being descriptive with your code is entirely the point. It's meant to be readable and avoid unnecessary cognitive loading of the future devs who come after you.

While latitude and longitude are topics you learn about in school, not everyone is going to retain that particular knowledge. It's especially not good to rely on people's vague recollection of lat and lon nuances they learned about in 3rd period math six years ago when their focus was on how they're gonna ask their crush to prom.

Well written code should be descriptive and readable by someone without domain knowledge and allow them to come away with the ability to describe at least the specific operations that block of code is doing and why. Named constants make that far easier a task than magic numbers. When you hire a new dev, and they look at the codebase, you don't want to have to spoon feed them the tribal knowledge about each class and function because they're written indecioherably.

[-]

codeprimate@reddit

“I am disappointed in your lack of professionalism. I hope that can help you achieve the standards your team and our organization expect from all contributors.”

[-]

dash_bro@reddit

My only viewpoint is: did we approach it softly, and is it a breaking change? If we answer no to the former, we know what we need to do. If it goes into a back and forth on the latter, it's always worth talking and aligning in person instead. Bickering on a pull request is unbecoming.

If it's not a breaking change, honestly I'd just let it go. It's not important enough for me to lose any sense of peace and sanity, although I agree readability goes up if you do what you suggested.

[-]

torwinMarkov@reddit

Yes, his attitude in his replies is totally arrogant. However 90 and 180 degrees sound pretty standard and not needing a constant, maybe. Would need more info.

Let me ask this — are the values used in more than one class (even in a test class)? If so you should extract. If not, I would make it optional.

[-]

Opposite-Hat-4747@reddit

Slightly frustrating? I’d be raising the issue with my manager. Not only is what you recommend such a basic good practice I can’t believe he’s refusing, the way he’s going about it is absolutely obnoxious.

[-]

kon-b@reddit

Report attitude to manager (assuming you're not one).

Response verbatim (assuming copy-paste unmodified) shows very toxic and culture destroying behavior.

Behavior adjustments are management problem.

[-]

DanceWinter5574@reddit

I joined a new project . I naturally write comments explaining everything( as i was new i was having hard time as there were absolutely no comments or documentation anywhere). They assumed my comments are copilot generated and asked me to clean them up. Now I intentionally write sloppy comments .

[-]

SureConsiderMyDick@reddit

I know that one of them is max 180, but i dont know nor care which one of the two is called latitude and longitude.

Hehe, you could joke about why he bring attitude to the codebase (in the comment)

[-]

ruiisuke@reddit

I'm lucky enough that my current team cares about readability/maintainability and no one bats an eye if you were to throw a PR back for magic consts. Being pissy in a PR comment would absolutely get me fired, which is saying something considering my team is relatively chill. Sorry this happened to you, OP. You were definitely in the right here.

[-]

dipshitwitha9toedwmn@reddit

Colleague is wrong and has never maintained a codename with a team where he didnt primarily write all the code. Just childish stuff.

[-]

hobbycollector@reddit

I mean, latitude only goes to ±90 ..

[-]

youngggggg@reddit

a moment of ego like this make you a pariah at my company in 2 seconds. You didn’t do nothing wrong and hopefully this dev does enough bs like this to get canned eventually because that is just not what being a team player looks like even if you strongly disagree with a comment

[-]

33ff00@reddit

Do people talk like resentful little bitches in fucking PRs where mgt could stumble across it our get reported for abuse? That is nuts.

[-]

rhd_live@reddit

No he had a little ego flair-up. Hopefully doesn't happen again, for his sake

[-]

Landon_Hughes@reddit

I’m so glad I’m out of this space

[-]

jedfrouga@reddit

“thanks for complying with basic style guides”

[-]

Otherwise_Source_842@reddit

Is this FAANG? Could see a NIT comment leading to anger on their end immediately as I have heard the amount of comments and revisions on PRs can be used as negative metrics for career review. Not saying the response the other dev gave is correct by any means just was pondering the logic behind the ego.

[-]

cuddle-bubbles@reddit

u come off as too nitpicky to me and I dont blame him for that

[-]

sherdogger@reddit

Sometimes it's gilding the lily if it's well known in context or among the team, but yah, there is no way that was a healthy and professional discussion, lol...

[-]

caboosetp@reddit

"I understand you don't see the value in this, but that will come in time as you get experience as a developer".

I have a vendetta against magic strings / magic numbers. People generally know this about me before i look at their PRs though.

If someone had reacted to me the way they did to you, I'd have had a meeting on their calendar to go over it.

[-]

UntestedMethod@reddit

Your feedback was correct. It's absurd that they said constants are "added complexity". Arguing to use magic numbers instead of constants suggests they are fairly inexperienced as a professional dev.

Their chatgpt comment is in plain bad form and shows a generally immaturity.

Overall sounds like a bit of an egotistical little cunt of a coworker. I'd message them privately to call out their chatgpt comment as completely uncalled for passive aggressive disrespect.

[-]

Sir_Edmund_Bumblebee@reddit

It’s always interesting hearing others’ perspectives. Personally I wouldn’t consider all numbers magic numbers. Like for example “timeInMilliseconds = timeInSeconds * 1000” would you consider 1000 to be a magic number here that needs to be pulled out as a constant? Some numbers are pretty intrinsic to the context they’re used in.

[-]

greensodacan@reddit

It upset you, which happens to the best of us.

This wouldn't have been a blocker for me because it doesn't actually break anything and it's not necessarily brittle or insecure. That said, I've see codebases like this where stylistic issues start to compound and become real problems: the Broken Windows Theory.

A diplomatic approach you can use in these cases, is to a) say if it's a blocker upfront, and b) list how committed you are to a change on a scale of 1-10. It helps the other person gauge their own emotions, and hopefully you can come to an agreement.

There's ego on both sides here. It happens. If it becomes a common occunance, go to your lead before you get too frustrated.

[-]

Sleve__McDichael@reddit

Another option is to use Github's suggestion feature and just make the correction yourself. It saves the original author's time, and lessens the impression that you're armchair reviewing.

depending on the coworker whose code you're reviewing, just making the change yourself can lead to a much stronger negative reaction than the one OP experienced here.

if someone feels the need to get this snippity/passive aggressive about review comments asking for clarification, they often become even more territorial and rude if you actually go in and make the change yourself (unless i'm misunderstanding your comment)

[-]

Free_Break8482@reddit

I think if you work regularly with latitude and longitude and other geographical data that this counts as domain knowledge and it's probably clearer to have the numbers in the code rather than a named variable. The 49th parallel is called the 49th parallel.

[-]

JohntheAnabaptist@reddit

Those aren't really magic numbers and in the cases of degrees like 90 and 180, there's a strong reason to think that he's right. Extracting those creates unnecessary obfuscation with the intent. In other cases you might be right, but without knowing more context, I'm leaning towards you're wrong on this one

[-]

dabup@reddit

I work with a guy like this and every suggestion he makes everyone incorporates because they're not really suggestions. They're kind of you should do it and they all make sense. But whenever someone has feedback for him, it's always no because this or oh thanks. But no, I'm not going to do that and we don't really have a way to enforce it and so we're stuck with letting him do whatever he wants. He's very immature and I think he also didn't go to college at all and I feel like that missed out on learning how to work in a team overall bad team player and I really don't like him

[-]

smootex@reddit

Are you junior to him?

I agree with your nitpick broadly but I will say, I had a dev junior to me coming up with insane nitpicks, clearly by the use of AI. It's really really been getting on my nerves, it comes across as them being annoyed when they have to address my comments so they're trying to come up with something to critique to get back at me or something. I'm not immature enough to leave that comment but there is a certain amount of internal stewing involved where I have to talk myself down from writing an essay about why the criticism isn't actually valid and if they had actually read the code instead of relying on AI they would understand why what they're suggesting is exactly the same as what I have implemented. What I'm trying to say is it might not all be about you, there's a certain amount of annoyance going around about some of these AI practices.

[-]

Fantastic-Duck4632@reddit

Yeah that guy sounds like he sucks. And is probably insecure.

[-]

Cherubinooo@reddit

Your colleague is correct on the merits but is still an asshole. 90 and 180 are not magic numbers in this context. They are fundamental to how latitude and longitude work, and it’s fair to expect anyone reading/writing code in this area to have a basic understanding of how lat/lng works. Extracting to named constants actually impedes readability.

That being said, calling you ChatGPT? Not cool

[-]

Agile_Government_470@reddit

Fuck this guy

[-]

MrIcedCafeMocha@reddit

Is your coworker a kid?

[-]

_msd117@reddit

Send them another review feedback to.improve the comments

[-]

grillaface@reddit

You’re right but it is also old school and not necessarily useful. Particularly if the values are only read once there isn’t much value in making a constant, and it’s arguably harder to read. Second if these 90 / 180 are obvious (like just dividing 360 etc) then yeah it’s kinda silly and cumbersome to make constants for them

Like if value = foo / Constants.halfACircleInDegrees did we gain anything?

[-]

Izkata@reddit

We would have in this case because when you saw 180 you seem to have misunderstood what the number means.

[-]

Insomniac1000@reddit

fuck him. document this shit and send it to his manager.

He's a piece of shit.

[-]

apartment-seeker@reddit

sorry that happened, but why is this a thread here

[-]

GludiusMaximus@reddit

I had a long back and forth with a colleague about stuff like this, and bad behavior aside, really the absence of stricter lint rules and a style guide is what got us there in the first place. There is no way resolve this when it’s a matter of taste and at least one person behaving badly. only resolvable if you can point to a previously established set of standards and prove it doesn’t meet them. otherwise it’s passive aggression till the cows come home

[-]

HQxMnbS@reddit

Crazy part is: dealing with comments like this has never been easier cuz an agent can do it instantly

[-]

PersianMG@reddit

For me a magic number is one that is hard to tell exactly what it means at a glance.

In the context of a coordinator system, 90, 180, 360 are all pretty standard and I wouldn't necessarily make them constants. Now if it was something like 97 degrees and it represented some special value on your system, then I'd make that a variable.

So in this particular example I could go either way.

[-]

No_Meaning_5697@reddit

You write a comment where you ask if he can explain why a magic number makes more sense to him. If he didn't get any value in that he should at least give an explanation that a TL would like to hear

[-]

PossibilityFun299@reddit

pip usually means they're letting you go

[-]

EstanislaoStan@reddit

Honestly I wasn't ever taught lat and longitude. Was surprised the other day that they have different ranges lol.

[-]

alchemyDev@reddit

I would’ve probably replied with a link to a Wikipedia about the magic number anti-pattern, and then said I thought most of us learned this in undergrad but I’m glad I had the opportunity to educate him on it

[-]

skymallow@reddit

Personal opinion is if you raise a nit in a PR you need to be prepared to just let it go. Otherwise it's just a nit.

But when teammates get snarky like this I just drop a "thanks boss man" and a prayer emoji.

It gives me a little satisfaction that either they can't tell I'm being sarcastic, or they can tell but can't prove it.

[-]

Educational-Lemon969@reddit

for me personally, I don't find it obvious that 180 should even be a valid value for a lattitude variable xD. intuitively, latitude should be from <-90;90> (0 for equator), assuming you use degrees ofc. that's why there should be a constant - not because constants should be dogmatically put everywhere, but so that you can put a doccomment next to it explaining all this.

also lat is a terrible variable name. assuming you don't use some kind of Degrees wrapper type, the next best thing is to at least put the unit in the name so that you're less likely to accidentally shoot yourself in the foot by passing it somewhere that expects radians. also it's 21th century, we have code completion so why hamper readability by saving characters?

[-]

Yoiiru@reddit

I think 90, 180 etc are ok if a comment was added above the if statement. Otherwise agree with Constants. Having constants and well named variables also helps the AI, because while a dev might know what 90 means due to business logic, the AI probably won't have that info

[-]

kbielefe@reddit

There are different schools of thought on this, but not every number is necessarily a magic number. If it's obvious in context, universally understood, only used in one place, and not going to change, it's okay and usually more clear to just put the literal.

51.5085 is a magic number and should be replaced with london.latitude. if abs(lat) > 90 in a function named validateCoordinates is a style choice. If it's repeated several times and people keep putting 180 there by accident, it goes back to needing a defined constant.

[-]

introspective_pisces@reddit

Don’t make a big stink about something like this. If it’s not something you’re accountable for then make the suggestion and move on. Keep it in your notes in case you need to refer back to it.

This is a borderline case anyway. It probably is obvious from context that 180 is a semicircle in degrees. Is it good style to put it in a constant? Sure, but practically speaking it’ll be fine.

If you make a thing out of it you risk giving the appearance of being difficult. If you let it slide one of two things happens: the guy uses good judgement and extracts constants where it makes sense and there’s no problem. The other is that they exhibit a trend of refusing reasonable feedback on their PRs and then it’s something you can bring up.

But give people some leeway at first, especially when you’re not sure they deserve it. That’s the foundation of kindness.

[-]

ProbablyNotPoisonous@reddit

It probably is obvious from context that 180 is a semicircle in degrees.

In this code snippet it's a latitude.

Ironically, this is exactly why it should be named.

[-]

bit_shuffle@reddit

Latitudes don't go to 180. Longitudes do. OP doesn't math much, and clearly has zero experience in mapping software/GNC.

[-]

mdrjevois@reddit

OP, it depends on the application, but there is a decent chance your colleague is right that "devs should know this". I'm sure your colleague did not include checks like "if lat > 180", though.

Latitude ranges in [-90, +90] -- from south pole to north pole. Longitude goes round and round, so there is no universal min and max, just local conventions like [0, 360] or [-180, +180]. And all of that is assuming degrees, but other units are possible. So depending on the rest of the codebase, either (1) degrees are established as default, or (2) "maybe rename to lat_deg?" would be a more appropriate nit.

Anyway, there is no guarantee that writing this in terms of named constants makes the code clearer. It depends. But you absolutely should know the basics of this coordinate system before putting your colleague on blast in this sub.

(Yeah maybe your colleague should've been more mature in their response as well)

[-]

nosayso@reddit

Right or wrong that's grossly unprofessional and worth taking note of if it becomes a pattern and the behavior should be escalated to their manager - which it probably will.

[-]

z960849@reddit

I had a similar argument with a guy who wanted to add 3 additional parameters to a 15 argument function. After a few back and forths on why it was a bad idea, I started having ChatGPT reply to his comments and it was much more vicious in its replies. It caused him to go ape shit and started blocking other people's PRs for nitpicks, before that he never reviewed anyone else's code.

[-]

starquakegamma@reddit

It’s a bit nit-picky - these are common angles, eg 180 degrees isn’t going to change. I would keep them as numbers.

[-]

theskillr@reddit

PR rejected - no magic numbers

repeat until its overridden or fixed

[-]

Few-Strawberry2764@reddit

As someone starting a software company, if I saw my devs doing this they'd get a stern one on one lecture. They keep doing stuff like that and I'd seriously think about firing them. It's not just petty and poor practice, it clearly shows you don't care about trying to produce a good product or take pride in your work.

[-]

6a6566663437@reddit

IMO for universal magic numbers like this, or hours in a day or similar, I wouldn't require a constant but I would require a comment if you're not using a constant.

'Cause it's not that weird for someone to do comparisons backwards, use the wrong direction for negative longitude, or similar logic errors. So I want them to write out something that indicates their logic either via a comment or a constant acting as a pseudo-comment.

For platform specific constants like "max connections" or whatever, that needs to be a constant.

[-]

bit_shuffle@reddit

You are wrong. Different choices of coordinate system can be made, [0,360), [-180,180). "MaxLongitude" doesn't tell me anything. 360 tells me something. 180 tells me something. You want to be able to understand the bounds on the range, especially in mapping, GNC, and similar applications. In this case, explicit numerical values help.

Your entire set of complaints, cognitive load, easier for the next dev... is actually arguing -against- named constants instead of explicit values in this case.

Your coworker went along with you, just to get you to go away. Your codebase is now more opaque because you made a consideration (named constants) a hard rule where it didn't fit.

[-]

coweatyou@reddit

I don't have a problem with them using a hardcoded constant, > 180 of a latitude is pretty obvious as to what numerical constant it is. I have a problem with hardcoding this type of coordinate.

Now, my background might be a bit unique (I deal with lots of different data types in different forms), but I would like to have this check functioned out to make it easy to adapt if unit type changes. Lat long degree float (-180.0, 180.0) can easily change to a lat long degrees min sec ((0,0',0"), (360,0',0")) or lat long degree with a radian based measure or a BAM. Having this check be an explicit function can catch to make it easy to adapt to different unit types.

Or, better yet, use a unit library and have it handle all of this automatically. But again this might all be over kill for whatever your use case is.

[-]

Banquet-Beer@reddit

If you ask me, 90 and 180 are pretty clear on the intent. You are being picky.

[-]

AftyOfTheUK@reddit

His comments would get him a reprimand from me. That's condescending, unprofessional and shows low standards. That's a bit of an orange flag, tbh

[-]

popovitsj@reddit

I replied explained my reasoning like self-documenting, thinking for future devs, less cognitive load etc… with examples on how it would look like.

That sounds really over the top. You shouldn't do that for a nitpick and depending on how that looked exactly, the snarky chatgpt comment may be understandable.

[-]

sudoku7@reddit

Cool, that changes this from a comment on the PR to a require change on the PR.

[-]

Foreign_Addition2844@reddit

Is this the first usage of 180/90 in this context? If so, then you should not make a constant "that can be reused", because it is the first time, and it is unnecessarily complex. If its been used a few times, then yes.

[-]

Puzzleheaded_Low2034@reddit

if abs(lat) > Constants.maxLatitudeOfThreeHundredAndSixtyDegrees { ... }

Compromise! /s

[-]

Gusatron@reddit

The solution is instead of giving them constructive feedback in the future, you fail it and leave a poo emoji.

💩

[-]

BaddDog07@reddit

Even if he disagreed other dev should have just done it, this is so small its not even worth pissing your coworkers off over

[-]

CrayonUpMyNose@reddit

Have they heard of radians, where angles are expressed in units of the distance on the surface divided by the radius? Using hard-coded constants for physical measurements is always a bad idea, just ask NASA:

https://www.cnn.com/TECH/space/9909/30/mars.metric.02/

[-]

Responsible-Hold8587@reddit

That NASA issue has literally nothing to do with consts.

Using consts is not dangerous. Using consts without including units is dangerous.

Obviously, consts are problem if you're doing something like const float32 MaxAngle = 90.0, but that's stupid.

Ideally, your const is defined as some strong type that cannot be used incorrectly, like const Angle MaxAngle = angle.FromDegrees(90.0) or const Angle MaxAngle = angle.FromRadians(pi / 2) and all your functions work with Angle instead of unit less primitives.

[-]

shan23@reddit

Would you ask for a constant for number of sides on a square, or sum total of degrees in a triangle?

[-]

Kamaroyl@reddit

Your colleague is wrong and has a bad attitude is my thought. Do you have a linter for your project? I recommend adding one, making sure there's a "no magic numbers" rule and blocking merge based off that passing as well as unit tests. Stops any of these conversations.

[-]

PettyWitch@reddit

That’s a good idea

[-]

LordFlippy@reddit

Wow sounds like a dick. I'd tell him you didn't use ChatGPT, just had a basic grasp of programming best practices.

[-]

HayatoKongo@reddit

Have seen this a lot over the past year. A lot of instances of overzealous and arrogant senior devs just suggesting anything they don't like is AI-generated. Not sure if it's paranoia or just laziness.

[-]

LordFlippy@reddit

Sounds like the type of cowboy devs that don't realize that having been at the right place at the right time doesn't make you a good engineer.

[-]

floofsnsnoots@reddit

This guy would fail a face to face interview with me in about 5 minutes flat. You work with a fool.

[-]

noelypants@reddit

The immature comment is the actual concern to me. I think a line has to be calmly-but-firmly drawn in the sand that everybody is expected to show up and act like they’ve been here before

[-]

aruisdante@reddit

Turn on readability-magic-numbers in clang-tidy and avoid the argument in code review.

Seriously, policies like this shouldn’t be debated in code review, it just leads to disagreement like this based on “style preferences” even if there are very good reasons not to have magic numbers outside of tests. Developers only argue about style if the tools let them. If the tools don’t let them, they light hate it, but they’ll just get on with their jobs.

[-]

Visa5e@reddit

Not directly related, but I'd create a Latitude wrapper class that validated the value on creation.

But yeah, your colleague is an arse.

[-]

dbxp@reddit

Google maps actually maxes out at 85 degrees lat due to how it maps the globe to a mercator projection

[-]

saposapot@reddit

It does seem like an overkill. Not using magic numbers doesnt mean you don't have any number in your code. When a number is a very well known constant I find it's easier to have it there instead of just a constant for constant sake.

I also agree this is common knowledge. If someone doesn't know this, they shouldn't be touching code related to it.

Having said that, that's now how a professional discussion should go. Even if you said the most dumb thing ever, that's not how we communicate.

I would address it with manager and nip this in the bud. That's not how a team should talk and work together. That's not how a review should be handled.

[-]

No_Software8474@reddit

The guy is being rude but also wrong. Assuming devs will know something years in the future is foolish. 180 could mean a lot of things and someone could easily fat finger a refactor and break things. Using an enum is overkill but having a simple constant called MAX_LATITUDE would work. The bigger issue and this is what separates really experienced devs from those that aren’t is being defensive about code. Nobody owns the code and if someone makes a reasonable suggestion that won’t break anything then there is nothing wrong with implementing it. They took time to review the PR thoroughly so no need to dismiss the comments and the chat gpt line was passive aggressive bs.

[-]

BonelessTaco@reddit

He sounds like an insufferable person, even though I’d probably let this slide myself

[-]

IdeaJailbreak@reddit

I would not block a PR on it, and not even mention it if it’s in a single file with comments or method names to provide the idea that you’re dealing with longitude or latitude.

Once it’s used in more than one place in the code base, then extract for ease of refactoring/lessen risk of incoherence.

[-]

i_dont_wanna_sign_in@reddit

For situations like these I give no options.If there's an anonymous constant (I'm a python guy) my comment says "CONSTANTS must be defined outside of function scope with concise naming". Depending on the relevant scope of the constant, the use-case, etc. I'll tell them to define an Enum or whatever the case may be.

When you're not breaking PEP standards then reviews are more flexible.

[-]

Cernuto@reddit

enum Degrees { OneHundredAndEighty = 180; }

[-]

Expert-Reaction-7472@reddit

reply with something like

"I didnt use chatGPT. Just put the fries in the bag bro."

[-]

iliketurtles69_boner@reddit

Leave a code review comment addressing their snarky comment along the lines of “thanks for adding the constants, but let’s keep comments to-the-point and professional”.

If they respond in any way other than removing the snark just bluntly ask them what benefit they bestow upon the codebase with such comments.

Honestly attitudes like the one you describe need to be snubbed out immediately. All developers should be thinking along the lines of “I personally don’t see the value in adding named constants, but it isn’t a negative in any practical way and someone has already said they would benefit from it - logic dictates I should just add it”.

Working with developers with misplaced egos is always part of the job. There really are just lots of arseholes out there. Unfortunately working with people who view code review comments as personal attacks is just a feature of this job.

Maybe they had a bad day. It’s certainly worth addressing and nipping in the bud. Probably not a hill worth dying on but equally needs pointing out. Stay firm but unflustered, don’t get emotionally drawn in.

[-]

mwax321@reddit

What a dickhead.

Add an automated PR rule for no magic numbers. Make a big deal out of it. Fuck him right back

[-]

PettyWitch@reddit

It’s a totally valid suggestion and no, you can’t expect devs to know latitude and longitude. 20% of adults in the US are functionally illiterate.

[-]

letsgoowhatthhsbdnd@reddit

their response to you seems very unprofessional. i would have pushed harder lol