Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a recommended style for float division #628

Closed
Drenmi opened this issue Feb 7, 2017 · 9 comments
Closed

Introduce a recommended style for float division #628

Drenmi opened this issue Feb 7, 2017 · 9 comments

Comments

@Drenmi
Copy link
Contributor

@Drenmi Drenmi commented Feb 7, 2017

By default, Ruby will perform integer division when both receiver and argument are integers. The way you perform float division on integers is to coerce one or more of them into a float. I propose there be a guideline for which part--receiver or argument--should take the #to_f coercion.

I am leaning towards both the receiver and the argument being coerced, because:

  1. If you only coerce one, changing the code gets more risky, because you may unwittingly replace the float coerced one with an integer, regressing to integer division.

  2. Choosing one to apply #to_f to is (as far as I can see) completely arbitrary.

Possible styles:

# both
a.to_f / b.to_f

# receiver
a.to_f / b

# argument
a / b.to_f

Another option could be to recommend #fdiv:

a.fdiv(b)

WDYT?

@backus
Copy link
Contributor

@backus backus commented Feb 7, 2017

Personally I'd prefer the default style to not be a form that requires #to_f on both sides. Since it isn't necessary it is dead code to require both.

Personally it seems like the preference should be to enforce that the left hand receives #to_f. They you always know you are dealing with Float#to_f. Not true if the right hand is the thing being coerced. Since it is ruby I still don't really know what / means for unknown type a 😄

@Drenmi
Copy link
Contributor Author

@Drenmi Drenmi commented Feb 7, 2017

@backus: Unless #to_f was overridden for type a (but let's not go there. 😅) I think your argument is sound, and you're well on your way to win me over. 😀

@GBH
Copy link

@GBH GBH commented Jan 21, 2019

Got burned very recently on this bug/feature. Was wondering if there was a cop that would prevent this. It's very easy to make 5/2 == 2 when you really expected 2.5. If you forget this gotcha you're in trouble.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 12, 2019

@Drenmi Let's recommend left-hand #to_f or fdiv. I think both are fine approaches, provided you stick to them consistently.

tejasbubane added a commit to tejasbubane/ruby-style-guide that referenced this issue Jun 12, 2019
@bbatsov bbatsov closed this in #756 Jun 12, 2019
bbatsov added a commit that referenced this issue Jun 12, 2019
koic added a commit to koic/ruby-style-guide that referenced this issue Jun 22, 2019
This PR updates a style guide for "Float Division".

The original issue for which this style was proposed.
rubocop#628

It is an update based on the following argument.
rubocop/rubocop#7153 (comment)

The following is a quote from the argument about the reason for this change.

> I wanted to show the following option to make a bad case only when both have `to_f`.
>
> ```ruby
> # bad
> a.to_f / b.to_f
>
> # good
> a.to_f / b
> a / b.to_f
> a.fdiv(b)
> ```
>
> Whether `to_f` is used on the left or right depends on the nature of `a`
> or `b` parameter. On the other hand, it is redundant that `to_f` is used
> in both. At that time, I think it is better to let users choose whether
> to remove the left side `to_f` or the right side `to_f`. This is the
> reason I recommend this option to default.
bbatsov added a commit that referenced this issue Jun 22, 2019
This PR updates a style guide for "Float Division".

The original issue for which this style was proposed.
#628

It is an update based on the following argument.
rubocop/rubocop#7153 (comment)

The following is a quote from the argument about the reason for this change.

> I wanted to show the following option to make a bad case only when both have `to_f`.
>
> ```ruby
> # bad
> a.to_f / b.to_f
>
> # good
> a.to_f / b
> a / b.to_f
> a.fdiv(b)
> ```
>
> Whether `to_f` is used on the left or right depends on the nature of `a`
> or `b` parameter. On the other hand, it is redundant that `to_f` is used
> in both. At that time, I think it is better to let users choose whether
> to remove the left side `to_f` or the right side `to_f`. This is the
> reason I recommend this option to default.
@morus
Copy link

@morus morus commented Sep 11, 2019

a = '1.1'
b = '2.1'
d = a.to_f / b.to_f

Does a rule that forces me to choose between turning it off or having broken code really make sense?
Of course in my real world sample a and b are not literals but parsed from xml ...

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Sep 11, 2019

Well, that's not really in the scope of the rule - it deals with coercion of integers to floats.

@morus
Copy link

@morus morus commented Sep 12, 2019

But It applies to any coercion. The rule does not check that it deals with integers. Understandable given the lack of explicit typing but that does not change the consequences...

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Sep 12, 2019

I think you're confusing the guidelines here with the RuboCop cop. The cop obviously can't know the type of the receiver and it might yield false positives, but that has little to do with what the guidelines are intended for. On a related note - when dealing with an external data it's better to use Float than to_f most of the time.

@morus
Copy link

@morus morus commented Sep 12, 2019

I guess you are right. Sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants