Skip to content
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

Jump to bottom

Pr 5 #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Pr 5 #5

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b4a2c2
is all unique elements function
CheezItMan Aug 8, 2019
429afb1
add export
CheezItMan Aug 8, 2019
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
14 changes: 14 additions & 0 deletions is_all_unique_elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

Copy link

Choose a reason for hiding this comment

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

Add a comment to describe what type of uniqueness you are looking for (unique characters? unique array items?)

const isAllUniqueElements = (array) => {
Copy link

Choose a reason for hiding this comment

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

Could we rename to areAllElementsUnique? Might read a little more smoothly ...

let isAllUnique = true;

array.forEach((currentElement, currentIndex) => {
Copy link

Choose a reason for hiding this comment

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

good function to check for unique elements

array.slice(currentIndex + 1).forEach((elementToCompare) => {
Copy link

Choose a reason for hiding this comment

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

Consider limiting the number of loops within loops to improve time efficiency

if (elementToCompare === currentElement) {
isAllUnique = false;
Copy link

Choose a reason for hiding this comment

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

I feel like we should be returning isAllUnique right away if it is false so that the code doesn't have to continue through the entire array before returning.

Copy link

Choose a reason for hiding this comment

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

You may want to consider exiting loop if isAllUnique==false. Otherwise, it will loop through the whole array.

Copy link

Choose a reason for hiding this comment

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

Returns true for empty array -- should it return null instead?

Copy link

Choose a reason for hiding this comment

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

If you add a return as soon as you find a duplicate, it will be more efficient because it won't have to keep searching through the rest of the array

Copy link

Choose a reason for hiding this comment

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

We can add a return if isAllUnique = false instead to make the program run more efficiently

Copy link

Choose a reason for hiding this comment

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

Can we refactor to exit the loop as soon as it is false?

}
});
});

return isAllUnique;
};
Copy link

Choose a reason for hiding this comment

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

Nice easy-to-read function