-
Notifications
You must be signed in to change notification settings - Fork 50
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 - Cyndi #32
base: master
Are you sure you want to change the base?
Sockets - Cyndi #32
Conversation
Portmanteau GeneratorWhat We're Looking For
Good job overall! I really like the way you experimented with breaking different functionality out into different methods. There were a few places where I saw room for improvement, which I've tried to call out inline below, but in general I am happy with this submission. Keep up the hard work! |
when "a", "e", "i", "o", "u" | ||
return "true" | ||
else | ||
return "false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should return true
or false
(the boolean value) here, rather than "true"
or "false"
(a string containing the word "true" or "false"). Getting comfortable with using different data types is an important thing to practice early on.
def rev_str(strg) | ||
aa = strg.split("") | ||
rev = Array.new | ||
aa.length.times do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use more specific variable names here?
rev_str
could bereverse_string
aa
could bestring_array
rev
could bereversed
IMO, that would make this code much easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, Ruby has a way to do this built in: String#reverse
puts "abcd".reverse
# => dcba
puts "What is the second word to combine?" | ||
b = gets.chomp.downcase | ||
b = check_length(b) | ||
b = check_contain_vowel(b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the idea of breaking these individual tasks out into separate methods - that makes the flow of this code very easy to understand. Good work!
If you wanted to go even further, you could combine these 4 repeated lines into another method, maybe something like get_user_input
.
if word.length < 2 | ||
until word.length > 1 | ||
puts "Your word must be 2 or more characters." | ||
word = gets.chomp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method would do exactly the same thing if you removed the if
statement entirely:
def check_length(word)
until word.length > 1
puts "Your word must be 2 or more characters."
word = gets.chomp
end
return word
end
def vowel_index(word) | ||
count = 0 | ||
word.each_char { |letter| | ||
if is_vowel(letter) == "false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I would prefer you use do..end
over {..}
. It makes the code a little easier to read.
def check_contain_vowel(word) | ||
count = vowel_index(word) | ||
if count == word.length | ||
until count != word.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, you don't need both the if
and the until
, since they're checking the same thing. If count != word.length
, the until
loop won't run at all.
Portmanteau Generator
Congratulations! You're submitting your assignment.
Comprehension Questions
is_vowel?
method affect your project?