Skip to content

declare constants#1

Open
susanreenesaa wants to merge 1 commit intomasterfrom
develope
Open

declare constants#1
susanreenesaa wants to merge 1 commit intomasterfrom
develope

Conversation

@susanreenesaa
Copy link
Copy Markdown
Owner

@susanreenesaa susanreenesaa commented Sep 6, 2019

@dennisja plz review the second challenge for week two.
qn
Write a function that takes a string and returns an array of length 2 containing a new
string made of all and only the vowels from the original string and the number of duplicates in the original string. Only the first instance of the vowel is considered.
For example:
countVowels(‘dahdah’) # will return (‘a’, 3)
countVowels(‘drink water’) # will return (‘iae’, 1)

@dennisja
Copy link
Copy Markdown

dennisja commented Sep 6, 2019

Hey Like I said on the first challenge, Put the question in the description. I don't have access to the question. I have to look at what the question to determine whether what you are doing is what the question wants or if there is a better way of doing it

Comment thread VowelsAndDuplicates.js
Comment thread VowelsAndDuplicates.js
// removing duplicate vowels from the string
const uniqueVowels = [...new Set(vowels)].join("");
// getting duplicates from the original string
const numberOfDuplicates = name
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job on this.
But we are splitting lucky enough using an empty delimiter which is O(StringLength), then sorting, then joining and matching. These can be expensive operations if we have very long strings, Can you think of a way you can count duplicates in a string without doing all those operations?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

working on it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great

@dennisja
Copy link
Copy Markdown

dennisja commented Sep 9, 2019

I noticed that you don't have tests for this challenge. Can you write unit tests for your code?

Copy link
Copy Markdown

@dennisja dennisja left a comment

Choose a reason for hiding this comment

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

Let's Just Add tests for this and get it merged

Comment thread VowelsAndDuplicates.js
// removing duplicate vowels from the string
const uniqueVowels = [...new Set(vowels)].join("");
// getting duplicates from the original string
const numberOfDuplicates = name
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great

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