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.

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

generate words starting with letter #42

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SuryanshuTomar
Copy link

@SuryanshuTomar SuryanshuTomar commented Jul 14, 2023

Summary

Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"

What are the specific steps to test this change?

For example:

  1. Run the website and log in as an admin
  2. Open a piece manager modal and select several pieces
  3. Click the "Archive" button on the top left of the manager and confirm that it should proceed
  4. Check that all pieces have been archived properly

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Work Done -
Created the function generateRandomWordStartingWith which will take a letter or a word and will return a random word from the list of word already present in the file. In case, an invalid input is given then, the function will return null.

Next PR - NA

Video -

indexjs-random-words-visual-studio-code-2023-07-14-20-28-07_nJ1iawRO.mp4

@SuryanshuTomar SuryanshuTomar changed the title Suryanshutomar/ #17 generate words starting with letter generate words starting with letter Jul 14, 2023
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

This feature should be added as an enhancement to the existing generate function's options, so that it may be combined with other options. Also there no need to document it as a restriction to just one letter, as indeed your code already works with a longer prefix if a longer prefix is passed.

So I recommend naming the new option prefix and integrating it into the existing logic, rather than making a more limited standalone function that can't be combined with other functionality.

@SuryanshuTomar
Copy link
Author

@boutell Here is the updated video -

recording-2023-08-20-124311_SGeuChJa.mp4

@BoDonkey
Copy link
Contributor

Howdy @SuryanshuTomar -
Your code is looking pretty good. There are a couple of more things you need to do, however. If you look at the PR template, we require that:
a) The changelog be updated. Don't assign a new version number. Instead, put it under a heading of "UNRELEASED"
b) Document how to use your new feature in the README.md file
c) Write tests (in /tests/test.js) to show that your feature works. You should have tests that pass for a variety of conditions, - returns a single word with a specific letter, returns 5 words all starting with the same letter, returns words of a minimum or maximum length starting with a letter. Plus at least one test where it fails - for example, what if the user wants a word of 8 letters that starts with 'z''?

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Thanks! We're getting close here but there are a few things to take care of. See my comments.

In addition:

  • Please add to the documentation.
  • Please add appropriate unit tests in tests/test.js.

let result = null;

// Filter the wordList to find words starting with the specified prefix
if (prefix) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make sure it's a string:

if (typeof prefix === 'string')

const randomIndex = Math.floor(Math.random() * filteredList.length);
result = filteredList[randomIndex];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

} else { (follow our style for if/else whitespace)

## UNRELEASED

Added Function generateRandomWordStartingWith()
- That will take a string an input.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a new changelog entry mentioning the new option.

const filteredList = wordList.filter((word) =>
word.startsWith(prefix)
);
if (filteredList.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Throw an exception (throw new Error('Unavailable')) if no words are available meeting the criteria.

@@ -1986,14 +1988,31 @@ export function generate(options) {
let rightSize = false;
let wordUsed;
while (!rightSize) {
wordUsed = generateRandomWord();
// Generate a random word with the provided prefix (if any)
wordUsed = generateRandomWord(prefix);
Copy link
Member

Choose a reason for hiding this comment

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

While it was already possible in this loop, adding prefix increases the likelihood that we'll never find a word and get stuck in an infinite loop, which is problematic for sure. So please refactor generateRandomWord to also take min and max and include them in your filter criteria. That way we will know if it's impossible to satisfy the request due to any combination of min, max and prefix and can throw one clean exception. And this "rightSize" while loop can go away.

Probably generateRandomWord should just be merged with this 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.

None yet

3 participants