feat: add decimal to fraction algorithm #1439

Please make clear what the spec of this is. What is the accuracy? If it's in decimal places, why do you need any complicated logic beyond rounding, and choosing the appropriate power of ten for the denominator?

Maths/Fraction.js Outdated Show resolved Hide resolved

MarufHasan24 added 2 commits

October 10, 2023 11:11

@MarufHasan24


 Update Fraction.js

2abb00d

@MarufHasan24


 fixing spelling Fraction.js

560127f

MarufHasan24

MarufHasan24 commented

Oct 10, 2023

View reviewed changes

Copy link

Author

@MarufHasan24 MarufHasan24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I have answered all your questions, and resolved those, what I think I should.

Maths/Fraction.js Outdated Show resolved Hide resolved

@MarufHasan24


 Update Fraction.js

82fd7fc

@appgurueu

Copy link

Collaborator

appgurueu commented Oct 10, 2023

I'd like to reiterate this:

Please make clear what the spec of this is. What is the accuracy? If it's in decimal places, why do you need any complicated logic beyond rounding, and choosing the appropriate power of ten for the denominator?

Code-wise, it seems you misunderstood my point. I'm asking for a "flattening" of the code using early returns / throws. The logic should be something like "if input is invalid, throw. if input is trivial (integer), return early. now do the actual meaty logic (if we got here, we didn't return or throw already)".

@MarufHasan24

Copy link

Author

MarufHasan24 commented Oct 11, 2023 •

edited

Loading

I'd like to reiterate this:

Please make clear what the spec of this is. What is the accuracy? If it's in decimal places, why do you need any complicated logic beyond rounding, and choosing the appropriate power of ten for the denominator?

Code-wise, it seems you misunderstood my point. I'm asking for a "flattening" of the code using early returns / throws. The logic should be something like "if input is invalid, throw. if input is trivial (integer), return early. now do the actual meaty logic (if we got here, we didn't return or throw already)".

If we got the number which is not an integer, then what!? Or if the is input is not a number even, then I will wait for what? Please clear me.😟

@appgurueu

Copy link

Collaborator

appgurueu commented Oct 12, 2023

If we got the number which is not an integer, then what!? Or if the is input is not a number even, then I will wait for what? Please clear me.😟

The logic should stay the same, it's just about how you write it. Currently the code is a mess of if-elses - e.g., elses to throw in error cases. What I'm proposing is to use early returns and throws to clean it up structure-wise. Suppose you have

function decimalToFraction(decimal) {
 if (typeof decimal === "number") {
 if (Number.isInteger(decimal)) {
 return [decimal, 1]
 } else {
 // this is where the logic for converting a valid decimal that isn't an integer to a fraction goes...
 }
 } else {
 throw "invalid type"
 }
}

then this would be better written as

function decimalToFraction(decimal) {
 if (typeof decimal !== "number") throw "invalid type"
 if (Number.isInteger(decimal)) return [decimal, 1]
 // this is where the logic for converting a valid decimal that isn't an integer to a fraction goes...
}

MarufHasan24 added 5 commits

October 13, 2023 06:56

@MarufHasan24


 Updated Fraction.js

be441e7

@MarufHasan24


 Update spelling in Fraction.js

1e2000d

@MarufHasan24


 Oh! spelling.😓

aa71162

@MarufHasan24


 Update Fraction.test.js

c33f80e

@MarufHasan24


 logical error fixed.

ca69ae3

@MarufHasan24

Copy link

Author

MarufHasan24 commented Oct 13, 2023

@appgurueu fixed all issues you have mentioned.

@MarufHasan24

Copy link

Author

MarufHasan24 commented Oct 30, 2023

@appgurueu any progress?!

@raklaptudirm

Copy link

Member

raklaptudirm commented Nov 8, 2023

@appgurueu waiting for your updated review.

appgurueu

appgurueu requested changes

Nov 10, 2023

View reviewed changes

Copy link

Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this robustly implementing (or even precisely defining) the problem yet; the code is also currently still quite dirty.

Maths/Fraction.js

* @function fraction

* @description This function returns the fraction of a float type number.

* @param {number} num - a float type number is expected, but an integer will also work.

* @param {number} accuracy - the accuracy of the fraction, the default is 6. like if the accuracy is 2 then for 1.333 result will be 10/9, where for 6 it will return 1333/1000.

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please formulate this mathematically - something like "fraction - num <= 10^-accuracy".

Maths/Fraction.js

* @see https://en.wikipedia.org/wiki/Repeating_decimal and

* @see https://en.wikipedia.org/wiki/Decimal#Decimal_fractions

* @example fraction(0.5) // [1, 2]

* @example fraction(0.3333333333333333) // [1, 3]

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all these examples really useful? 0.5 and 0.25 for example seem redundant.

Maths/Fraction.js

* @example fraction(0.33,2) // [10, 3]

function fraction(number, accuracy = 6) {

if (typeof number === "number" && !Number.isNaN(number) && Number.isFinite(number)

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still not using guards here; these checks are all redundant with the if-throws in the else. You just need to move the if-throws to the top of the function, making them guards.

Maths/Fraction.js

len /= div;

return [neg * number, len];

} else {

if (typeof number !== "number") throw new TypeError("Invalid number, a number type value expected");

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These "guards" should all be moved to the top of the function. They could also be simplified a bit.

Maths/Fraction.js

} else {

if (typeof number !== "number") throw new TypeError("Invalid number, a number type value expected");

if (typeof accuracy !== "number") throw new TypeError("Invalid accuracy, a number type value expected");

if (Number.isNaN(number)) throw new TypeError("Invalid number, a number type value expected");

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NaN check is redundant with the negated isFinite check.

Maths/Fraction.js

len = 10 ** (number.length - 1);

number = Number(number);

}

// it will find out the gcd of the number and the len to reduce the fraction nomitor and denominator like 4/8 will be 1/2

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called "shortening" the fraction. number and len are odd names for "numerator" and "denominator".

Maths/Fraction.js

"9".repeat(reg[1].length) + "0".repeat(rec.length - pos[0].length)

);

} else {

// if number is not a repeating decimal then following code will run

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again a redundant comment.

Maths/Fraction.js

} else {

// if number is not a repeating decimal then following code will run

number = number.replace(".", "");

len = 10 ** (number.length - 1);

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not apparent to me why this is correct: Isn't number.length the length of the whole number (without the period, in digits), not just the "fractional" part after the period here?

Maths/Fraction.js

);

} else {

// if number is not a repeating decimal then following code will run

number = number.replace(".", "");

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String manipulation for manipulating numbers is dirty here. Just multiply your numerator by the denominator, then round. Shouldn't the denominator be determined by the accuracy?

Maths/Fraction.js

// if number is a not an integer then following code will run

number = number.toString();

let len;

let reg = number.match(/(\d+?)1円+$/);

Copy link

Collaborator

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hacky way to deal with "repeating" fractional parts, and I'd argue that it solves the wrong problem. Since we're dealing with finite-size numbers (floats) here, there are no "repeating" decimals; we can't distinguish a "repeating" decimal from a "deliberately ending decimal". We need to treat both the same (it seems "accuracy" is more of a "threshold" of when you consider a decimal repeating?).

In fact, all floats can even be represented exactly as fractions (though perhaps maybe not as fractions of floats, that might get hairy towards some edge cases) where the denominator is a power of two and the numerator is an integer.

I wouldn't exclude the possibility that some special provisions for repeating decimals may be useful, but then what problem this exactly solves (and how) and what guarantees it makes should be explicitly stated.

@MarufHasan24 MarufHasan24 closed this

Apr 11, 2025

Labels

None yet

4 participants

@MarufHasan24 @appgurueu @raklaptudirm @RRMovies

Uh oh!

feat: add decimal to fraction algorithm #1439

feat: add decimal to fraction algorithm #1439

Uh oh!

Conversation

@MarufHasan24 MarufHasan24 commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your change:

Checklist:

Uh oh!

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@MarufHasan24 MarufHasan24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

appgurueu commented Oct 10, 2023

Uh oh!

MarufHasan24 commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

appgurueu commented Oct 12, 2023

Uh oh!

MarufHasan24 commented Oct 13, 2023

Uh oh!

MarufHasan24 commented Oct 30, 2023

Uh oh!

raklaptudirm commented Nov 8, 2023

Uh oh!

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

@appgurueu appgurueu Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@MarufHasan24 MarufHasan24 commented Oct 5, 2023 •

edited

Loading

MarufHasan24 commented Oct 11, 2023 •

edited

Loading