Skip to content

adding a new method #73

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.

Sign up for GitHub

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

Open
vlad-ko wants to merge 2 commits into
base: main
Choose a base branch
from
Open

adding a new method #73

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e82648a
adding a new method
vlad-ko Jun 9, 2025
667fb53
adding factions
vlad-ko Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions resources/js/classes/calculator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ class Calculator {
squareRoot(number) {
return Math.sqrt(number);
}

absolute(number) {
return Math.abs(number);
Comment on lines +27 to +29
Copy link
Contributor

@codecov-ai codecov-ai bot Jun 9, 2025

Choose a reason for hiding this comment

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

Input validation is missing in the absolute method. You should verify that the input is a valid number to prevent unexpected behavior.

Suggested change
absolute(number) {
return Math.abs(number);
absolute(number) {
if (typeof number !== 'number' || isNaN(number)) {
return NaN;
}
return Math.abs(number);
}

}
Comment on lines +28 to +30
Copy link

@seer-by-sentry seer-by-sentry bot Sep 26, 2025

Choose a reason for hiding this comment

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

Consider adding input validation to ensure the parameter is a number. The current implementation will pass through any input to Math.abs(), which may produce unexpected results for non-numeric inputs like strings or objects.

Suggested change
absolute(number) {
return Math.abs(number);
}
absolute(number) {
if (typeof number !== 'number' || isNaN(number)) {
throw new Error('Input must be a valid number');
}
return Math.abs(number);
}

Did we get this right? 👍 / 👎 to inform future reviews.


factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
Comment on lines +32 to +34
Copy link

@seer-by-sentry seer-by-sentry bot Sep 26, 2025

Choose a reason for hiding this comment

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

The factorial method should handle non-integer inputs more explicitly. Currently, it will attempt to calculate factorial for decimal numbers, which may not be the intended behavior. Consider checking if the input is an integer.

Suggested change
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
factorial(n) {
if (!Number.isInteger(n) || n < 0) return NaN;
if (n <= 1) return 1;
return n * this.factorial(n - 1);
}

Did we get this right? 👍 / 👎 to inform future reviews.

return n * this.factorial(n - 1);
Comment on lines +31 to +35
Copy link
Contributor

@codecov-ai codecov-ai bot Jun 9, 2025

Choose a reason for hiding this comment

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

The factorial method uses recursion which can lead to stack overflow for large inputs. Consider using an iterative implementation for better performance and to avoid potential stack overflow errors.

Suggested change
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
return n * this.factorial(n - 1);
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
let result = 1;
for (let i = 2; i <= n; i++) {
result *= i;
}
return result;
}

Comment on lines +31 to +35
Copy link
Contributor

@codecov-ai codecov-ai bot Jun 9, 2025

Choose a reason for hiding this comment

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

The factorial function lacks a check for large inputs which could cause performance issues or stack overflow. Consider adding an upper limit.

Suggested change
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
return n * this.factorial(n - 1);
factorial(n) {
if (n < 0) return NaN;
if (!Number.isInteger(n)) return NaN;
if (n > 170) return Infinity; // JavaScript's maximum safe factorial
if (n <= 1) return 1;
return n * this.factorial(n - 1);
}

Comment on lines +27 to +35
Copy link
Contributor

@codecov-ai codecov-ai bot Jun 9, 2025

Choose a reason for hiding this comment

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

Missing JSDoc comments for the new methods. Adding documentation would improve code readability and maintainability.

Suggested change
absolute(number) {
return Math.abs(number);
}
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
return n * this.factorial(n - 1);
/**
* Returns the absolute value of a number
* @param {number} number - The input number
* @returns {number} The absolute value
*/
absolute(number) {
return Math.abs(number);
}
/**
* Calculates the factorial of a non-negative integer
* @param {number} n - The non-negative integer
* @returns {number} The factorial value, or NaN for invalid inputs
*/
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
return n * this.factorial(n - 1);
}

Comment on lines +27 to +35
Copy link
Contributor

@codecov-ai codecov-ai bot Jun 9, 2025

Choose a reason for hiding this comment

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

Consider adding unit tests for the new methods to ensure they work as expected with various inputs, including edge cases.

}
Comment on lines +32 to +36
Copy link

@seer-by-sentry seer-by-sentry bot Sep 26, 2025

Choose a reason for hiding this comment

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

The recursive factorial implementation has potential performance and stack overflow issues for large values of n. Consider adding input validation for the upper bound and potentially implementing an iterative version for better performance and stack safety.

Suggested change
factorial(n) {
if (n < 0) return NaN;
if (n <= 1) return 1;
return n * this.factorial(n - 1);
}
factorial(n) {
if (typeof n !== 'number' || !Number.isInteger(n)) {
throw new Error('Input must be a valid integer');
}
if (n < 0) return NaN;
if (n > 170) throw new Error('Input too large, would cause overflow');
if (n <= 1) return 1;
// Iterative approach for better performance
let result = 1;
for (let i = 2; i <= n; i++) {
result *= i;
}
return result;
}

Did we get this right? 👍 / 👎 to inform future reviews.

}

module.exports = Calculator;