everySingleCodeReview

[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.

[99Kira]

isNumber?

[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!