Skip to content

Conversation

@yavorski
Copy link
Contributor

No description provided.

@yavorski
Copy link
Contributor Author

yavorski commented Oct 13, 2018

Hello

I noticed that the smallestElement which was found with reduce function is equal to 0 which is not correct, because the notSortedArr does not contains 0.

The smallest element in this array is 1.

The bug is caused because of the initial value given to the reduced function.

I rewrote the 2 declarations using Math.min and Math.max which are more concise and fewer locs.

Best of luck!
Yavorski

@codecov
Copy link

codecov bot commented Oct 13, 2018

Codecov Report

Merging #224 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #224   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         143    143           
  Lines        2554   2554           
  Branches      422    422           
=====================================
  Hits         2554   2554

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d12638...aef8f8a. Read the comment docs.

const biggestElement = Math.max(...notSortedArr);

// Detect smallest number in array in prior.
const smallestElement = notSortedArr.reduce((accumulator, element) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set smallestElement to 0, which is not correct.
The smallest element in this array is 1.

@MaksymMalin
Copy link

@lazarljubenovic
Copy link

0 is wrong there indeed but why would you put 1 instead?

The correct starting values for the reduce way of calculating extrema are Infinity for the minimum and -Infinity for the maximum.

@yavorski
Copy link
Contributor Author

@lazarljubenovic not putting 1 at all, this is just the answer in this particular case with this specific array in mind. See diff for more details.

@trekhleb
Copy link
Owner

@yavorski , good catch. Thank you for PR.

@trekhleb trekhleb merged commit 6bd6072 into trekhleb:master Oct 17, 2018
harshes53 pushed a commit to harshes53/javascript-algorithms that referenced this pull request Dec 6, 2018
shoredata pushed a commit to shoredata/javascript-algorithms that referenced this pull request Mar 28, 2019
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.

4 participants