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

Sockets - Shirley #30

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

Sockets - Shirley #30

wants to merge 2 commits into from

Conversation

shirley1chu
Copy link

@shirley1chu shirley1chu commented Feb 8, 2019

Portmanteau Generator

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
What went well in your code style, such as indentation, spacing, variable names, readability, etc.? What was lacking? I think my code is quite readable, but I'm not sure if it's the most concise or efficient. I tried to create a method that would return the index for the desired vowel for word 1 and word 2, but I don't know if it's more efficient than having separate loops for word 1 and word 2, or if the method is written clearly enough. I am concerned that if I had to check whether my word contained a large selection of letters (not just vowels), it would lead to longer run times that might crash the program.
How did creating the is_vowel? method affect your project? Since I had to validate while a word had vowels or whether a specific letter was vowels, the is_vowel? method came in handy, and saved me from repeatedly typing a long command (not sure if command is the right vocab word to use here).
What was your strategy for getting the correct set of letters from the first word into the portmanteau? I use a method to iterate through each letter, starting from the last letter. It would return the index of the last vowel, which I then used to slice the first word to generate the portion needed for the portmanteau. Since I excluded the indexed letter from the portmanteau portion, I created a special if loop to address the case that the indexed letter is the last letter and is also a consonant (in that case, that letter should not be excluded).
What was your strategy for validating against inputs under 2 characters? I created a method that had an until loop to prompt the user to re-enter the word if the word length was under 2 characters. I also tested the program and entered one-character words in multiple rounds to make sure my validation loop worked.
In the next project, what would you change about your process? What would you keep doing? I would make sure that I understand the requirements of the program correctly (in this case, rules behind creating the portmanteau). I would also aim for more efficient and concise code.

I understood the portmanteau rules retaining everything before (but not including) the first vowel of the first word, and everything after (and including) the first vowel fo the last word
@dHelmgren
Copy link

Portmanteau Generator

What We're Looking For

Feature Feedback
Readable code with consistent indentation Your indentation is great, and I appreciate that you put your helper methods first in the file; it gives good context for what's happening in the body.
Practices using variables appropriately Yes
Practices using conditionals appropriately Yes
Practices iteration appropriately Yes
Practices using custom methods appropriately Yes
Program validates against input under 2 characters Yes
Takes in two inputs and creates a portmanteau Yes, but it breaks if you add in rAndom CapItal LettErs

Copy link

@dHelmgren dHelmgren left a comment

Choose a reason for hiding this comment

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

Good work!

@@ -0,0 +1,51 @@
def is_vowel?(letter)
return ["a", "e", "i", "o", "u"].include?(letter)

Choose a reason for hiding this comment

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

You miss upper case characters here! one way to fix this is using letter.downcase()


# method to find the index of desired vowel, whether it be first or last vowel
# char_idx represents index for the letter that you want to start iterating at
def find_vowel_index(word, char_idx, count_direction)

Choose a reason for hiding this comment

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

I like that you broke this out into it's own method

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