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 - Sarah #43

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

Sockets - Sarah #43

wants to merge 2 commits into from

Conversation

sjscotton
Copy link

@sjscotton sjscotton 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 thought that my code was refactored fairly well, with decent variable names and readability. Where I think it is lacking is in how verbose my methods for chopping the first and second words. Both of these could have been much more concise. EDIT As of 2/9 I committed my code again after making the chop_word methods more concise.
How did creating the is_vowel? method affect your project? This method was great refactoring step ensuring I didn't have to write code to test if a letter was a vowel twice.
What was your strategy for getting the correct set of letters from the first word into the portmanteau? My strategy was to use the each_char method on the reverse of the first word, this would ensure that the first vowel I found, would be the last in the word. Because i saved the index value of this vowel, i was later able to slice the first word based on that.
What was your strategy for validating against inputs under 2 characters? I choose write a loop that would continue to ask the user for input, until the length of the word provided was at least 2.
In the next project, what would you change about your process? What would you keep doing? Next time, I would continue to focus on getting the code to work properly first, and then focus on refactoring and editing to make it more readable and understandable. Next time I would try and outline my code first to help organize my thoughts / write pseudocode.

@dHelmgren
Copy link

Portmanteau Generator

What We're Looking For

Feature Feedback
Readable code with consistent indentation Yes! Especially good use of methods to build up functionality.
Practices using variables appropriately Usually, see comment
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



def get_word_from_user(word)
puts "What is the #{word} to combine?"

Choose a reason for hiding this comment

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

word as a variable on its own is weird here. My own (admittedly fiddly) opinion is that it's not descriptive enough. Maybe word_num, or cardinal?


def get_word_from_user(word)
puts "What is the #{word} to combine?"
word = gets.chomp.to_s

Choose a reason for hiding this comment

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

I would also advise against reusing this variable again for a different purpose! Part of your job is to convey the semantic meaning of your code with variable names, so using word to mean both the cardinality of the word (first second etc.) and the word that we fetch from the user is confusing!

end


def chop_second_word(second_word)

Choose a reason for hiding this comment

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

I bet you could find a clever way to combine chop_first_word and chop_second_word

second_word = get_word_from_user("second word")

first_half = chop_first_word(first_word)
second_half = chop_second_word(second_word)

Choose a reason for hiding this comment

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

This this beautiful! Great use of methods!

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