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

Fix issue #291 - add implementation to WordleEC.guess(String guess) #347

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RongjingH
Copy link

Add implementation to WordleEC.guess(String guess) for wordle-kata-solutions module #291

Copy link
Contributor

@rzrobin213 rzrobin213 left a comment

Choose a reason for hiding this comment

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

Thanks for tackling one of the harder issues and congrats on coming up with a working solutions. I really like the way you thought about the problem and commented the code; I think you're on the right track. For our Eclipse Collections Katas, we want our solutions to use the rich APIs provided by the Eclipse Collections (EC) library in their solutions. I added some comments about how to transform your solution in a more EC way.

.idea/codeStyles/Project.xml Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not commit files in the .ideas directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not commit files in the .ideas directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not commit files in the .ideas directory.

@@ -15,7 +15,7 @@

public class WordleECTest
{
// @Test // Uncomment once guess is implemented for WordleEC
@Test // Uncomment once guess is implemented for WordleEC
// @Tag("SOLUTION")
Copy link
Contributor

Choose a reason for hiding this comment

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

Uncomment the Tag for the solutions module too. We set up the project so this tag is needed for our builds to run.

return guess.toUpperCase();
}

for (int i = 0; i < guess.length(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the design goals of Eclipse Collections is to provide functional and fluent APIs to process and use the collections that it provides. There are many built-in methods for MutableCharBags (especially methods to help with iteration) that you can leverage instead of writing a for-loop (hint: collectWithIndex and injectIntoWithIndex). This makes the code more readable and abstracts away many of the complexities of the code. We encourage contributors of our EC katas to use these methods in their solution. So try to solve this without using a for-loop or while-loop :)

if (!guessBag.contains(hiddenChars.charAt(i))) {
hiddenBag.removeOccurrences(hiddenChars.charAt(i),hiddenBag.occurrencesOf(hiddenChars.charAt(i)));
}
//Delete all char which is not contained by hiddenBag in guessBag
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you're finding the characters that are different between the guessBag and hiddenBag and storing them into one bag. Perhaps you can just use one of the iterators built-in the CharAdapter to build a single bag to keep track of the differences. You can add to the bag when the characters are present

MutableCharBag hiddenBag = hiddenChars.toBag();
MutableCharBag guessBag = guessChars.toBag();

char[] resChars = new char[guess.length()];
Copy link
Contributor

Choose a reason for hiding this comment

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

Use collections provided by Eclipse Collections!

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

2 participants