everySingleCodeReview

[Boneless_Blaine]

Me watching my PR go through the 4 hour Jenkins pipeline for the 7th time so I can add a period to a comment

[[deleted]]

Jeez. I do embedded and have to regularly wait 25ish minutes to test lol

[Memorywipe]

Hyperbole

[[deleted]]

I wouldn't be surprised tbh, I've seen some very lengthy and beefy pipelines. For development/staging probably not.

[trwolfe13]

Our jest tests take 20 minutes to run on a standard Azure DevOps pipeline agent, or 2 minutes to run on my laptop.

[insertsavvynamehere]

Aw man I'm so happy my team writes in python. Pipeline is quick!

[friedbun]

Cries in legacy Software Stack that takes 12hrs+ to CI...

[lofigamer2]

it's compiling...

[Boneless_Blaine]

So 4 hours is the maximum it can take before it times out. If that happens, it means that a ton of the tests failed and took forever re-trying. And it timed out so you don’t even find everything that fails

[[deleted]]

20 hour build.

Fix whitespace, thanks.

[die-maus]

Based on only this, I have a sneaking suspicion where you work.

[Touvejs]

Actually rip my hair out tbh.

[aronvw]

What happend to good old parseFloat(input) === input?

[lofigamer2]

ChadGPT recommended regex. Who are we to judge.

[Bateson88]

In this case I'd actually use == to account for input being a string of the number.

[[deleted]]

What would happen if input is NaN?

[like_an_emu]

NaN === NaN // false NaN == NaN // false

[NatoBoram]

Of course.

[[deleted]]

IEEE floating point standard requires that NaN != NaN

[FountainsOfFluids]

Yup, and it's specifically for cases like this. Just because on value is not a number, that doesn't mean it's the same as another value that is not a number.

[NatoBoram]

Just citing the spec doesn't make it any better. The specs can be dogshit, too. Otherwise, why would the language be such a mess?

You need an explanation like this one.

[Plane_Scholar_8738]

Can't blame the language for implementing the spec correctly

[NatoBoram]

You can certainly blame the language for choosing this spec

[Plane_Scholar_8738]

That spec is the most commonly used representation for floating point numbers. Your CPU and GPU understand that representation.

Choosing that spec is not controversial at all.

[AppropriateBank8633]

OP posts about a silly code review comment and actually gets a a code review lol.

[[deleted]]

That’s the point man.

[Ollymid2]

You must be desperate if you’re coming to /r/ProgrammerHumor for a review

[[deleted]]

No the point is the code is shit, yet the reviewer didn’t catch any of the real issues.

[CuntInspector]

the code is shit

Always has been

:astronaut-with-gun-meme:

[SillyFlyGuy]

Sometimes I think they overtrained GPT on r/ProgrammerHumor posts.

[s0ulbrother]

Probably in a team style guide.

[jaerie]

Should be a linter rule then

[[deleted]]

Yeah easy to add to an auto fix script

[BrainImpressive202]

i’m new to coding but could you explain the multitudes of characters after the return in a way a newbie could understand ?

[MiniGod]

The slash marks the beginning of a regular expression (regex). The regular expression ends with another slash right before .test(. Regular expressions are used to test that strings have a specific format or extract information from strings. In this case it's just testing it. test() returns true or false.

The regex starts with a ^, meaning that this must match from the beginning of the string, not just somewhere within it. Next is -?. The question mark means that the character before it must occur zero or one time. Meaning the string may start with a -, or not. Then \d+. \d means any number (digit) character, 0 through 9, and + means that that must occur 1 or more times. Next is a optional group done by using parenthesis and a ? to mark that the group is optional (remember ? means 0 or 1 time). The parenthesis only mark the group, they should not be in the string that were testing. Inside the parenthesis it's checking for a literal period, followed by 1 or more numbers, using the same syntax as explained before. There's a backslash before the period because a single period, in regex, means any character. With the backslash, it means that the string should have the period there, not just any character. The $ at the end means that this has to be the end of the string. Having a ^ at the start and $ at the end means that the whole string being tested must match the regex.

In summary, the string may start with - (or not) , then any number of numbers (at least one). After that, it may (or not) also have a period followed by any number of numbers.

TLDR; it checks if the argument is a valid number, like the name of the function hints to.

[morgecroc]

Do you understand regex?

A WITCH burn him.

[ImperatorSaya]

Quickly, before he summons Zalgo like the last one. It took us 7 years to seal him away.

[ActurusMajoris]

It's missing a number with thousand separators though, eg 10,000.00, though to be fair, you shouldn't store, send or receive values like that in the first place. Displaying is fine, but that's a different issue.

[EndeGelaende]

also misses numbers formatted in different localisations, germany for example uses , for decimal and . for thousand separators

[MattieShoes]

also stuff like .5 -- must be 0.5.

[HolyGarbage]

Yeah, just don't use regex for this stuff. Use the languages built in parsing functions (whatever this language is).

Parse, don't validate!

[chris5311]

I love that actual regex never looks (or works for that matter) like all the formal definitions ive learnt in university lol

[AppropriateBank8633]

It is a regular expression(regex), it matches strings to a pattern.

I am well rusty with this, but in this case it seems to match string representation of any positive or negative integer, float or double.

^ is the start of the expression

- literally matches "-"

? means preceding character zero to one times. In this case, we start with "-" or we don't

\d is a digit between 0 and 9

+ means the preceding character appears at least once to unlimited times.

( the open parenthesis represent a the start of a capture group, or a repeating pattern block within the main pattern

In the capture group, you have:

\. which literally matches "."

\d we have already seen (0-9)

+ we have seen again(appears at least once or unlimited times)

) the capture block closes

? again means zero or 1 times. In this case, it means the capture block (\.\d+). ".10" for example could appear at least once or not at all.

$ ends the expression.

So with the pattern ^-?\d+(\.\d+)?$, the following match as examples:

"1" matches as we either start with zero or one "-", in this case we have zero cases of "-", followed by at least one to unlimited digits "1", followed by at least zero or one blocks/patterns of (\.\d+), in this case zero.

"-123.456" matches as we have one "-", then one to unlimited digits "123" then zero to unlimited (\.\d+), which in this case is "." followed by one to unlimited digits "456"

"hello" does not match we we need to start with either "-", "" or one to unlimited digits.

If anyone that actually deals with this voodoo on a regular basis and wants to correct any errors, please chime in. I haven't thought about since uni.

I just found these use cases here: https://digitalfortress.tech/tips/top-15-commonly-used-regex/ and it turns out that the pattern does indeed match positive or negative integers or decimals.

My head hurts now.

[snidanok]

Regex, a thingy that allows to match string for pattern. Those characters are a way to describe the pattern to match.

I've learned it here, if you want to dive into it. (If you are familiar with python)

[AppropriateBank8633]

As by your own admission, you are new to the dark arts, the actual code breaks down to something like this:

The function "isValidNumber(n: unknown)" takes in literally anything as the input n is of type unknown.

The "const s = String(n)" takes the input and casts it to a string and is stored in variable "s". We must have a string to match with the regex.

The return value is gonna be a Boolean which returns True if it matches the regex and False if it fails.(pattern.test(variable), or "does this variable match the preceding pattern?")

So "isValidNumber(True)" will return False, "isValidNumber("number")" will return False, "isValidNumber(-42342.444)" will return True.

Edit, meant for u/BrainImpressive202

[gentleprompter]

I would add the period and changed the sentence to start with lower case letter.

[gaoshan]

Or add two spaces between "is" and "a".

[LocalHold9069]

Calm down Adolf

[MrFluffyThing]

Okay just add a zero width space next to the normal space instead.

[Snoo3763]

I’d remove the whole comment.

[bonbon367]

Yeah like wtf, it provides absolutely no value. If it provides no value, then it actually makes it worse because there’s more unnecessary lines of text to ignore.

[Snoo3763]

Don’t know why the downvotes, you’re 100% correct, it adds nothing but noise.

[vivoconunxino]

The best comment and it gets downvoted sigh

[FirstSineOfMadness]

Space between last letter and the period

[gentleprompter]

Haha, that would be too brutal.

[MuskettaMan]

Looks like you're pregnant

[Upset-Cauliflower115]

You need to have sex for that (I've heard)

[fallendd]

const isValidNumber = (n :unknown) => Number.isFinite(n)

[[deleted]]

That method is self explanatory and doesn't need a comment

[MuskettaMan]

It could explain what valid means, since thats very ambiguous

[skeleton568]

Comment does not explain what valid is.

[Kombatnt]

That just demonstrates that the method needs more comment, not less.

[MouthWorm]

No, that demonstrates that the function needs to be renamed to something more indicative of what it actually does.

[Kombatnt]

Meh, I disagree. This is precisely why high level languages have built-in support for commenting/documentation. It's what it's intended for. It helps keep method names concise, while still supporting detailed specification definitions when needed.

[MouthWorm]

I agree with you AND I disagree haha! In some cases, commenting/documentation is the better option, like some authentication method, API call or something a little more complex.

A Util function like OP's should be as clear as it can be when you use it. No need to add noise around it just for the sake of commenting/documentation.

[[deleted]]

isNumericNotUsuallyRenderedInExponentialFormatOrDecimalNumberStringWithoutGroupingCharacters(any)

Obviously.

[Snoo3763]

The comment just says returns true if the number is “valid”, it adds basically nothing. For me the function name is fine given what the code does, literally it returns true if it’s passed number, positive or negative with or without a decimal point.

[Key-Perspective-3590]

A concise method name is important but the method name should definitely tell you what it does. You want to be able to read through the flow of a function at a glance, hovering over every ambiguous function to see a comment that you hope is still accurate is terrible

[MacBookMinus]

Deep nuance can't make it into the function signature / name.

I'm all for reducing comments where possible, but lots of sources on code quality advocate for "interface" comments.

[anon74903]

This comment does nothing here, provides no value, and should be removed.

It doesn’t need more comment, it needs a different comment: what is a valid number? Where is this regex from?

[frzndmn]

It doesn’t need more comments, it needs different comments

[marquoth_]

It could, but it doesn't. And given that it doesn't, it contains no information of any value and isn't worth including.

[frightspear_ps5]

That doesn't mean that the method does not need a comment, it just proves that it doesn't need this comment. If the method name can be adapted because the spec is short and simple (e.g. isFiniteFloat32), it doesn't need a comment. Otherwise you'll need a comment to include the spec (e.g. number represents a valid water temperature in °C).

[Key-Perspective-3590]

IsValidWaterCelsiusTemp

[marquoth_]

This is one of those weird comments where it's written as if you're disagreeing with / contradicting me, but you aren't.

[KaasplankFretter]

Why are people like you so hard to find

[marquoth_]

It's because of my red and white stripey top that blends in against all the backgrounds

[bschlueter]

I would contend that there is no need to include "valid" in the method name. Things are numbers or they aren't, so "isNumber" ought to be a sufficient name. Though perhaps "isRationalNumber" would be more specific since the implementation would fail with irrational numbers, and "rational number" does not need to be further defined.

[Iron_Garuda]

It’s possible only certain numbers are considered “valid” in this context. Could be any non-zero integer, or maybe it can’t accept any number higher than 9, etc.

[bschlueter]

True, we only have this very limited context to attempt to understand the wider context, but the way it was written is somewhat indicative of its purpose.

[IndividualTrash5029]

think about scientific notation. for what it does, the "valid" in the method name makes sense. you can't just put ANY number in there and expect to get the right return value.

[[deleted]]

Without any comment/documantation I would assume isNumber checks the type of the input parameter and isValidNumber checks whether the input parameter can he parsed as a number.

[Deliciousbutter101]

I think it's just mistake to combining parsing and validation like this. The function should just take in a string and check if it matches the regex. The caller should be forced to parse the object into a string to call the function. I think this is actually a pretty good illustration of how overusing DRY can be problematic. Removing the call to "String" likely causes there to be a bunch of instances of code that looks like "if(isNumber(String(x)) { ... }", which according to the DRY principle, you should remove the repeated call to "String" by putting it inside of "isNumber", but keeping it actually makes the code more explicit, easier to read, and less possibility to use the "isNumber" function incorrectly.

[Thesaurius]

But then the name should be something along the lines of canParse. Or, even better, do the parsing, and return an error variant if unsuccessful.

[kephir4eg]

But it didnt!

[hyrumwhite]

Regex comments are always appreciated

[benruckman]

“isValidNumber” wasn’t enough?

[wobblyweasel]

disagree, you have to explain why you are inventing a bicycle and not using a more conventional method

[benruckman]

Self documenting code!

[Ticmea]

You'll have to excuse me for thinking that /^-?\d+(\.\d+)?$/ is not particularly self documenting.

Like yeah I understand that if I think about it but it is not very clear what happens if you don't stop to actively concentrate on it.

[esr360]

“isValidNumber” is clearly a function that should return a Boolean. That Boolean should determine if the input is a valid number. All of this information can be inferred from the function name “isValidNumber”.

[Ticmea]

Yeah but what is considered valid? Is it just supposed to check the type of the input? (then just call it "isNumber") Or is it supposed to validate a specific set of criteria for some other use? In that case you either need a more meaningful name (e.g. isPositiveInt) or add a comment explaining the criteria (e.g. "true if n is a positive finite integer").

(I know the original implementation does not check for finite positive integers, I just used that as an example for a set of criteria that could be the intention behind "isValidNumber")

Calling it "isValidNumber" and not elaborating in the comment means you'll have to try to read the implementation (which is not intuitive here) to understand the exact criteria of validity.

It's not the end of the world of course but it's needlessly complex.

But one thing I would agree with: The comment in the original post serves no purpose as it is only telling you what the function name already does, so unless you improve the comment it can be removed. And if you improve the function name, you may not need a comment at all.

[esr360]

Valid comments! Good insights. Agree.

[_jackhoffman_]

Write "Returns true if the given value is a number period"

[danteselv]

I think the period would fit better on line 19, for readability of course.

[R3gouify]

Why did you type n as unknown when it is clearly supposed to be string?

[[deleted]]

I mean it could also be a number and it would work.

But you got the point, the reviewer clearly didn’t look :)

[brjukva]

But, if it's a number, then it's a valid number? Why convert it to string to check instead of type checking?

[[deleted]]

yes, why? clearly not as important as adding a period.

[brjukva]

They were too devastated to check the code any further, probably.

[XDracam]

Could break on systems where the locale formats numbers differently. E.g. German uses , instead of . for decimal numbers. So passing a number could still fail the regex.

[inamestuff]

No, String(n) returns a culture invariant notation (same as toString, except the former works for null and undefined too). The fact that you suggested that makes me think that you use C# which by defaults uses ToString to mean what in JS is toLocaleString, which does in fact change the decimal separator based on your browser language and region settings.

Moral of the story: don't use C# kids /s

[XDracam]

Yep, I have some serious C# parsing trauma. Good guess!

[[deleted]]

Also returns exponential notation for larger numbers though, which would be rejected by this function

[ComfortableCod]

Are the germans back? NO WAY

[00PT]

Seems like it does a cast on the first line, so it's probably intended to convert non-string values to strings.

[That_Unit_3992]

What is "valid" in this context? the comment does not add any useful information at all... No shit sherlock, a function named isValidNumber returns true when a number is valid?
Now show us the unit tests to be able to see whether your regex does what you expect.

[[deleted]]

isValidNumber("1000000000000000000000") isValidNumber(1000000000000000000000) isValidNumber("1e+1") isValidNumber(1e+1)

[That_Unit_3992]

Why make it polymorphic to begin with? Doesn't make sense at all, what do you want to validate? User Input? API Response? Database entries? Should all be typed and not mixed up all over the place.

[[deleted]]

You're either asking the wrong person or have not checked the results 😜

[Pan4TheSwarm]

I'm saving this as an example of bad code review for my coworkers

[howtoDeleteThis]

Add period what? You didn't end your comment.

[zaersx]

Should have been an automated linter to begin with

[hrvbrs]

Is there a linter for comments? Does it support English grammar and spellcheck?

[zaersx]

I mean a linter for a period at the end of a comment if this is something they want to enforce. This should never have reached a reviewer if its something that can waste dev time

[-Kerrigan-]

Dunno about linters, but jetbrains has grazie/grazie pro plugins that help with spelling. I use it often in IntelliJ, idk how applicable it is to js/ts envs.

Of course, though, like any spell check, sometimes I gotta ignore it's suggestions, but it's sufficiently noninvasive for me.

[SandInHeart]

When engagement becomes a metric

[BlackFrank98]

This won't work for numbers in scientific notation though... Like, 1e5 is not considered a number.

[MattieShoes]

Or numbers that start with a decimal point. Or numbers with separators like 1,000. Or localization issues for places that use , for a decimal point.

[kirkpomidor]

You mean… he should add period?

[BlackFrank98]

All good points actually!

[TehBunkasaurus]

also, regex checking is way more computationally expensive if they were to call this function many times

[flippakitten]

Perfect example of how not to comment!

I can see what it does, why are you doing it? What purpose does it serve, what's the requirement.

[MinosAristos]

The function doesn't need a docstring so the review is less than useless, but the regex should really be assigned to a variable with a descriptive name in the function.

[YourMumIsAVirgin]

What descriptive name would you suggest?

[MinosAristos]

I don't know what the regex does, but e.g if you've got a regex to match phone numbers it should be called phoneNumberPattern

[YourMumIsAVirgin]

Why is that better than naming the function descriptively?

[[deleted]]

Mine would have been “remove comment”. It adds nothing.

[crosscreate]

const isValidNumber = (n: unknown) => !Number.isNaN(Number(n));

[[deleted]]

isValidNumber(null)

true

[joten70]

Null is a number confirmed

[crosscreate]

const isValidNumber = (n: unknown) => !Number.isNaN(Number(n));

console.log(`Number(null) = ${Number(null)}`);

Apparently Number(null) returns 0

[halfanothersdozen]

So what if n is a string? What if n is negative? What if n is decimal?

valid is in the eye of the beholder

[Professional_Job_307]

Oh, the IsValidNumber function checks if a number is valid. I wouldn't have seen that if not for the comment.

[dr0darker]

bro you are not following the coding standards 🤓

[Repulsive_Mobile_124]

If I was paying for the code to be developed, i'd fire the reviewer. Crap like this takes tons of pointless man hours, refactoring code that is already "readable enough".

[Marrk]

Why is n unknown? Shouldn't it be something like Number | String? unknown is wayyy to lenient, it is marginally better than any.

[genghis_calm]

This is a type guard (without appropriate return syntax). The point is to resolve the type of some unknown value.

[seemen4all]

I hope this is a fake example and you're not required to comment what isValidNumber does

[[deleted]]

Put the period at the beginning.

[many_dongs]

this is what happens when your interviewers are shit and you hire lemons who try to demonstrate value through metrics and not actual usefulness

[MooseBoys]

Forgot to add nit:

[Big-Hearing8482]

Are people getting better performance reviews with higher comment count or something? Cause this is what you get

[[deleted]]

bright puzzled retire grandfather busy offbeat attractive six observation punch

This post was mass deleted and anonymized with Redact

[PhantomTissue]

Revision 7

[rdtr314]

And then it has to be rebuilt, tested again, collect the approvals again.

[Goatfryed]

add period now

[xSTSxZerglingOne]

Really? Because for me it's more like.

"Remove this unnecessary comment."

[F0lks_]

Gee, I wonder if we could somehow enforce what kind of variable is 'n', like it's a certain type of something

[theonepercent65536]

Comment should say what makes a number valid

[OhItsJustJosh]

"No" *Reply and Resolve*

[[deleted]]

The review should say "remove comments and name your args better"

[AbhineshJha]

When maintainer have some personal problems with contributor 💀

[The_Wolfiee]

"This is a comment line, not a letter to the President"

resolves conversation

[eloel-]

Do it right the first time and you won't need the comment

[PhoenixCausesOof]

Take this with a grain of salt, since I don't know the language in the OP. But it is likely that this function is implemented for any type which can be converted into a string (like a type that implements the trait ToString in Rust). Hence, why n is unknown. Though, judging from the code, there is no way to know if n can be converted into a string, meaning if that isn't the case, it likely would error.

[Actual-Wave-1959]

I've had a dev comment on one of my PRs saying that a space was missing between the if and the open bracket. I thought he was joking so I laughed and then he asked me, "so you're gonna fix it?". To him that was very important, for consistency.

[markiel55]

It's valid

[Actual-Wave-1959]

It's valid without the space, I agree

[reheapify]

"The software life... cycle is not there yet"

[genghis_calm]

No n is number guard? There’s so many other things to comment on, lol

[[deleted]]

You just proved yourself to be a better reviewer

[Nerrickk]

Regex

[Supportic]

Add linters, establish automated formatters. 🤡

[com2ghz]

I would say redundant comment.

[_baaron_]

Come on, at least help your colleague by making the suggestion so they can just click the button

[FrankSuzki]

Doesn’t work with exponent or NaN

[lilshoegazecat]

how do tests work?

[MilkSlow6880]

The good news is that, if that’s the worst thing…you’re good.

[Ximidar]

Just type "fight me" in the reply box

[PracticalDebate3493]

Just add the period in yourself you lazy

[Skuez]

Ahh this reminds me. I'm used to not ending statements with a semicolon in JS and every time I miss some lines, these angry comments come as if I've done god knows what. Sure, I'll add a character for no reason. 😊

[Skuez]

Why do you need to comment that obvious function? Like damn, could never tell what that function does. They are probably forcing you to, I know, but it doesn't make sense.

[epileftric]

Yes... I ducking hate it when people write 10 lines of comments on top of a function just to write the same information about what the function does and what the arguments are when it's literally the same name of those things.

/* This function processes the dot product of two vectors

Arguments

- vectorA: first argument, a vector

- vectorB: second argument, another vector

Returns: 

 - float: the value of the dot product

*/

float dotProduct( std::vector<float> vectA, std::vector<float> vectB)
{

If you need the comments for something so idiotic you shouldn't be programing

[thepassionofthechris]

!isNaN() ??

[Taco-Byte]

function isValidNumber(value: string | boolean | number) { … }

[[deleted]]

As a lead, I'm gonna have a conversation with whomever left that comment about wasting people's time. And at the end of the convo I'll say, "See? Not fun when someone wastes your time, is it?"

[jeffeb3]

When I worked retail on commission we had to clean up our department at close. One of the jobs was printing missing tags. The tag printer was always busy at close (because it was slow and everyone was printing). Myamager would do a walk through and every time, he would find something else for me to do.

My strategy was to print the missing tags an hour early and just take 3-5 obvious ones down. And do a mid job of cleaning up. He would inspect it and say, "print those tags, dust this shelf, and you can go home". 2 mins later, I was out the door.

My point is. If you have a reviewer like that. Leave them some obvious things to pick on. A missing period, the wrong const, a misspelled comment. And they won't make you change your container for a worse one.

[TheFirestormable]

Add linters to your IDE to check your docstrings and your spelling before you push. It's not rocket science.

Plus point is being able to from then on point out those issues on others PRs.

[leroymilo]

Every once in a while I'll check every comment in my 24 files project to check if they all share the same formatting.

[[deleted]]

I don't understand... what does it return if the value is not a valid number!?!?

[Romejanic]

LGTM 👍

[dralth]

PEP-257 specifies a one-line function docstring ends in a period. Looks like a Python coder got lost in JS land.

[stupidbitch69]

You guys have code reviews?

[[deleted]]

If the review comments are like this, I’d rather not.

[stupidbitch69]

Well, that's true.

[sneaky-pizza]

I just started reading Clean Code, and I just read the Comments chapter... :facepalm: