Welcome to the Treehouse Community
Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.
Looking to learn something new?
Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.
Start your free trialKushagra Patel
7,740 PointsCan someone review my code ?
Is it good enough or anything that I am missing to effectively do to the task.
def disemvowel(word):
abc=[]
new_list = list(word)
for iterable in word:
#print(iterable.upper())
if iterable=='a' or iterable=='e' or iterable=='i'or iterable=='o'or iterable=='u'or iterable.upper()=='A'or iterable.upper()=='E' or iterable.upper()=='I'or iterable.upper()=='O'or iterable.upper()=='U':
new_list.remove(iterable)
abc=''.join(new_list)
return abc
4 Answers
Steven Parker
231,275 PointsJust enter the code into the challenge and the validator will tell you if it does the job. And it does!
But I see two easy functional improvements you can make (it will pass either way):
- the first line isn't necessary, and a bit confusing to assign "abc" with a list but later as a string
- the assignment of "abc" with the joined list can be done just once, after the loop finishes
Brendan Whiting
Front End Web Development Techdegree Graduate 84,738 Points- when you're looping through
word
, you're calling the local variableiterable
for each iteration. But this thing isn't the iterable,word
is the iterable (a thing that lets you iterate through it) and we're iterating through the word. So it's a misleading variable name. Might be better to call it something likeletter
-
abc
is the name you're giving to the word with vowels removed. But I think you could give it a clearer name, likevowelsRemoved
- checking the letter against each vowel in lowercase and uppercase in a series of boolean checks is fairly inefficient and also hard to read because it makes a very long line (PEP8 style guide recommends 79 characters or less for line length ). Worst case scenario is 10 operations each time. We can do it more efficiently by having the vowels in a dictionary, and checking if a value is in a dict is O(1). We can also convert the vowel to one standard - let's say lowercase - before the check, that way we don't have to bother checking against the uppercase set of vowels.
- you're joining all the current contents of
new_list
intoabc
in every iteration of the loop, but this is unnecessary. You only need to do it once at the end.
Kushagra Patel
7,740 PointsThanks a lot Brendan! I really appreciate your brief feedback some of the things you mentioned went above my head I will try to learn about them.
Henrik Christensen
Python Web Development Techdegree Student 38,322 PointsJust a suggestion, but instead of using that long chain of or maybe try something like this
vowels = ['a', 'e', 'i', 'o', 'u']
for letter in word:
if letter.lower() in vowels:
new_list.remove(iterable)
This would make the code easier to read :-)
Maybe lower() 'word' before loop start - that way you don't need to run lower() more than once
Kushagra Patel
7,740 PointsThanks Henrik, I really appreciate your feedback!!
Rahul Saini
4,611 PointsI checked your code, My advice is just to keep things simple. Check my code
def disemvowel(word):
abc=""
for i in range(0,len(word)):
x = word[i].lower()
if x!='a' and x!='e' and x!='i' and x!='o' and x!='u':
abc += x
return abc
print(disemvowel("This is Just a String for Vowels"))
Kushagra Patel
7,740 PointsThanks Rahul, I really appreciate your feedback!!
Kushagra Patel
7,740 PointsKushagra Patel
7,740 PointsThanks Steven I really appreciate your feedback!!