It'd be easier to work with the Fraction objects if they typing was stricter and we had an interface.

As mentioned in the related issues, there are some things we could improve:

- Numerator and denominator could be typed as integers. They're already stored in the database as such and in the config as well.
- As we're using bcmath, the fraction values could be hinted to string, not float.

CommentFileSizeAuthor
#20 3100039-17_20-do-not-test.txt785 bytesdeaom
#20 3100039-20.patch10.43 KBdeaom
#18 3100039-12_17.diff6.66 KBdeaom
#17 3100039-12_17.diff6.66 KBdeaom
#17 3100039-17.patch10.41 KBdeaom
#12 3100039-12.patch9.39 KBdeaom
#8 3100039-8.patch9.29 KBkbrodej
#2 typehints_and_interface-3100039.patch6.65 KBzvonimirr

Comments

pcambra created an issue. See original summary.

zvonimirr’s picture

zvonimirr
he/him
Primary language Serbian
Location Subotica
commented
StatusFileSize
new typehints_and_interface-3100039.patch6.65 KB

Here is a patch I've put together.

zvonimirr’s picture

zvonimirr
he/him
Primary language Serbian
Location Subotica
commented
Status: Active » Needs review
pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Needs work

Thanks!!

Patch seems to be missing FractionInterface.php file?

*I think* numerator/denominator should not be casted to string.

Not sure why tests are failing, but there are some coding standard notices that can be reviewed in https://www.drupal.org/pift-ci-job/1533969

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

The only question pending here, probably for @m.stenta is whether we want the numerator and denominator as ints or strings.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Patch seems to be missing FractionInterface.php file?

I think this is why the tests are failing. I get Fatal error: Interface 'Drupal\fraction\FractionInterface' not found in /var/www/html/modules/fraction/src/Fraction.php on line 8 if I try to run fraction_from_decimal(0.5);

@zvonimirr00: Do you have a FractionInterface.php file that you forgot to include in the patch?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

The only question pending here, probably for @m.stenta is whether we want the numerator and denominator as ints or strings.

I think we need to work under the assumption that this Fraction class is working with strings as much as possible.

Currently the class accepts either strings or integers in setNumerator() and setDenominator(), but it always stores them as strings internally (casting them before saving them to $this->numerator and $this->denominator). It also is flexible in the types that are accepted in fromDecimal(). This is intentional.

Strings are used consistently through the class because it uses BCMath to perform arithmetic operations (when it is available, falling back to standard PHP int/float arithmetic if not). BCMath functions work with strings, so if we store numerator and denominator as integers internally, they will need to be cast to strings before being used in BCMath operations. Also, the methods toDecimal(), gcd(), and bcRound() all return strings. Again, because the assumption is that the class is using BCMath. Some decimal values are not representable as floats, so strings provide more possibility.

So, I think we should keep the numerator and denominator represented as strings internally. And we should keep the flexibility of accepting multiple types in setNumerator(), setDenominator(), and fromDecimal(), and not limit to string or integer.

I think that means the scope of this issue's changes will be more limited. Hopefully that doesn't negate all of the benefits you were hoping for with stronger typing @pcambra. What do you think?

kbrodej’s picture

kbrodej commented
StatusFileSize
new 3100039-8.patch9.29 KB

Hi. As the interface from previous patch was missing, I started from scratch. I followed the comment of @m.stenta from #7. As values are always stored as strings I did not typehint signatures, but I added typehints in doc-comment.

kbrodej’s picture

kbrodej commented
Status: Needs work » Needs review
pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Postponed

Thanks @kbrodej, let's see what happens with #3110374: Transform the constructor from decimal to be static because it affects this one.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Postponed » Needs work

@kbrodej @zvonimirr00 reopening this because the constructor went in, want to continue working on it?

deaom’s picture

deaom commented
Status: Needs work » Needs review
StatusFileSize
new 3100039-12.patch9.39 KB

Re-rolled the patch.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Needs work

@DeaOm thanks for the rerroll!

Some nitpicks below

  1. +++ b/src/Fraction.php
@@ -5,31 +5,36 @@ namespace Drupal\fraction;
+   * @param int $numerator
...
+   * @param int $denominator
...
+  public function __construct(int $numerator = 0, int $denominator = 1) {

    I don't think we want to enforce numerator/denominator 

  2. +++ b/src/Fraction.php
@@ -98,13 +91,7 @@ class Fraction {
+   * {@inheritDoc}

@@ -140,25 +127,16 @@ class Fraction {
+   * {@inheritDoc}

    In all these this should be "{@inheritdoc}" instead to follow standards 

  3. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+ * Defines interface for Fraction class.

    "the interface" 

  4. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+ * @package Drupal\fraction

    I think we're doing @package fraction everywhere else 

  1. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Set the numerator.
...
+   * Set the denominator.

    Sets 

  2. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Get the numerator.
...
+   * Get the denominator.

    Gets 

  3. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Calculate the decimal equivalent of the fraction.

    Calculates 

  4. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Calculate the fraction's greatest common divisor using Euclid's algorithm.

    Calculates 

  5. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Reduce the fraction to its simplest form.

    Reduces 

  6. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Reciprocate the fraction.

    Reciprocates 

  7. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Add another fraction to this one.

    Adds 

  8. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Subtract another fraction from this one.

    Substracts 

  9. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Multiply this fraction with another one.

    Multiplies 

  10. +++ b/src/FractionInterface.php
@@ -0,0 +1,157 @@
+   * Divide this fraction by another one.

    Divides

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

A big question to @m.stenta, can we assume that numerator and denominator are always int on the Fraction object? they're stored as int

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

I don't think we want to enforce numerator/denominator

I agree with @pcambra. Changing the constructor so that it does not allow strings is a backwards-incompatible change that could affect downstream code.

And: I think it's OK for the constructor to accept both strings and ints, because generally speaking PHP's automatic type conversion from string to int is pretty straightforward. It does not have the same issues that floats can have. (Please anyone with more knowledge of potential type issues correct me if I'm overlooking something!)

Other things I see:

  • The FractionInterface that is added in the patch is explicitly stating that strings or ints are acceptable to pass to setNumerator() and setDenominator(). So restricting to ints in the constructor is inconsistent. Changing the constructor to allow both would resolve this inconsistency.
  • The patch is declaring the protected $numerator and protected $denominator properties as @var string. That conflicts with both the constructor and the interface, as described above. Changing this to allow ints as well would resolve this inconsistency.
  • In FractionInterface, getNumerator() and getDenominator() both declare that they are returning strings. They may also return ints. The docblock should be updated to reflect this.
  • FractionInterface needs to be updated to reflect the changes from #3110374: Transform the constructor from decimal to be static:
    • It includes the old deprecated fromDecimal() method, but not the new createFromDecimal() method.
    • The docblock for fromDecimal() appears to be correct in FractionInterface, but it needs to be removed/replaced from Fraction.php with {@inheritdoc} like all the others.
    • The docblock for fromDecimal() in FractionInterface needs to include the deprecation notice.

Hopefully that's everything! I will give it another thorough review once these changes are made to make sure we didn't miss anything.

Thanks everyone for helping get this organized!

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Also (super small nit pick): looks like the patch is adding an extra newline under the constructor. I don't think that should be there (unless there's a new standard for that that I'm unaware of?)

deaom’s picture

deaom commented
Status: Needs work » Needs review
StatusFileSize
new 3100039-17.patch10.41 KB
new 3100039-12_17.diff6.66 KB

Hi @pcambra and @m.stenta, here is the updated patch and interdiff. I think I corrected everything that was mentioned in the comments.

deaom’s picture

deaom commented
StatusFileSize
new 3100039-12_17.diff6.66 KB

Removing the interdiff and adding it again but this time without testing it.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

Thanks @DeaOm!

Just one thing on my side:

  1. +++ b/src/FractionInterface.php
@@ -0,0 +1,171 @@
+   * @param string|int $value
+   *   The numerator value.
+   *
+   * @return Fraction
+   *   Returns this Fraction object.
+   */
+  public function setNumerator($value);
+
+  /**
+   * Sets the denominator.
+   *
+   * @param string|int $value
+   *   The denominator value.
+   *
+   * @return Fraction
+   *   Returns this Fraction object.
+   */
+  public function setDenominator($value);

    Could we call these variables $numerator and $denominator? $value is misleading IMHO

As a side note, I always post interdiffs as .txt files to avoid triggering the testbot, another option is to include "do-not-test" as part of the filename, see https://www.drupal.org/node/332678

deaom’s picture

deaom commented
StatusFileSize
new 3100039-20.patch10.43 KB
new 3100039-17_20-do-not-test.txt785 bytes

Hi @pcambra, updated patch and interdiff. And thanks for the do-not-test tip, appreciate it.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

This is looking good to me, leaving @m.stenta the final word :)

  • m.stenta committed 9fd4fa1 on 8.x-1.x authored by kbrodej 
    Issue #3100039 by DeaOm, kbrodej, zvonimirr00, pcambra, m.stenta:...
m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks @DeaOm and @pcambra! This looks good to me!

The only thing that jumped out is that the default parameter values for the constructor were removed. I assume that was a misunderstanding of earlier feedback. We do want to keep those defaults, so I restored them. If there was a reason for removing them, let me know.

Since there were many hands in this process, I'm giving Git's multiple co-author feature a try: https://stackoverflow.com/questions/7442112/how-to-attribute-a-single-co...

@kbrodej will get first author credit for contributing the first full patch including the interface file. Everyone else will be co-authors.

Thanks again everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.