Skip to content

Create getArrayMaxValue function#16

Open
Godelvatai wants to merge 2 commits into
buwebdev:mainfrom
Godelvatai:add_array_max_value_function
Open

Create getArrayMaxValue function#16
Godelvatai wants to merge 2 commits into
buwebdev:mainfrom
Godelvatai:add_array_max_value_function

Conversation

@Godelvatai

Copy link
Copy Markdown

Created getArrayMaxValue function and setup test cases for the new fucntion

Created getArrayMaxValue function and setup test cases for the new fucntion

@jbb-codes jbb-codes left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The logic for finding the max value looks correct overall, but there is a bug blocking all tests from passing.

Bug — src/utils/getArrayMaxValue.js, line 13

isArray is a static method on the Array constructor, not an instance method on arrays. This throws a TypeError on every call, including valid inputs.

// Current (broken)
if (!arr.isArray()) {

// Fix
if (!Array.isArray(arr)) {

Minor Issues

  • Both files are missing a newline at the end of the file. Most editors and linters expect one.

Suggested Improvements

  • Handle empty arrays — if [] is passed, Math.max() returns -Infinity. Consider adding a check after the Array.isArray validation and a corresponding unit test.
  • Handle arrays with no valid numbers — if every element is a non-number (e.g., ['a', 'b', 'c']), the filter strips everything out and Math.max() again returns -Infinity. Consider validating the filtered result before calling Math.max and adding a unit test for this case.

What's Working

  • The Math.max with filter approach correctly handles non-number values and zeros.
  • Test coverage is solid — all four cases are meaningful.

Please fix the Array.isArray call — once corrected, all 4 tests should pass. Requesting changes until then.

Fix to the Array.isArray function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants