6
\$\begingroup\$

I wasn't sure where to post this, but I'm just looking for a brutally honest critique of a program that I built. I'm new to coding and am taking my first course on it. I'm just trying to determine if my code is awful and I should consider a different career path, or if it looks like I'm heading in the right direction. This code is a short program we were required to write that had to accomplish the following at a minimum:

  • Accept an input from the user
  • Analyze the input for specific criteria that we determine, such as total length, specific characters present or not present, etc.
  • Must print feedback based on the criteria back to the user

My code is as follows:

# Greets user and names program
print("\n")
print("=Word Analyzer=")
print("This program will analyze a word that is provided by the user and provide some facts about it.")

# Queries user for input

word = input("Please enter your favorite word:\n")

# Converts input to all lowercase

new_word = word.lower()

# Creats spacing for formatting

print("\n")

# Prints total letter count of word

print("The total length of your favorite word is:", len(new_word), "characters")

# Creates string for beginning letter and converts to uppercase for formatting

beg_letter = new_word[0]
new_beg_letter = beg_letter.upper()

#Creates a string for the last letter and converts to uppercase for formatting

las_letter = new_word[-1]
new_las_letter = las_letter.upper()

# Prints what letter the word starts and ends with in uppercase

print("Your favorite word starts with the letter:", new_beg_letter, "and ends with the letter:", new_las_letter)

# If statement for determining if initials are present in the word provided by user

if ("s" and "n" in new_word):
    print("Your favorite word contains BOTH letters that make up my initials, S and N.")
else :
    print("Your favorite word does NOT contain BOTH letters that make up my initials, S and N.")
\$\endgroup\$
1
  • \$\begingroup\$ in case: you odn't need to do print("\n"), print("") print an empty new line. \$\endgroup\$
    – Je Je
    Commented Apr 25, 2022 at 20:22

2 Answers 2

8
\$\begingroup\$

Bug

This test does not do what you think it does:

if ("s" and "n" in new_word):
   print("Your favorite word contains BOTH letters that make up my initials, S and N.")

Rather, it's interpreted as:

if True and ("n" in new_word):
    …

Variable naming

The way you name variables as new_… is a bit annoying. In my mind, they're not "new", but just uppercase versions of the original. I'd recommend uppercasing the input as soon as you read it, because there's no point in working with lowercase here.

Avoid shortening variable names in weird ways. beg_letter is bad; las_letter is even worse. If you really want your variable names to have the same length, how about init_letter and last_letter? But really, you don't need to create variables for values that you only use once.

Printing

There are several ways to incorporate computed text into fixed strings in output. Using commas is one of my least preferred. I suggest using str.format(), or in more modern Python, f-strings.

Comments

Commenting nearly every line actually makes the code harder to read. Since the print() statements already make the code self-explanatory, I'd actually prefer no comments at all. The suggested program below is so much less cluttered!

print("\n")
print("=Word Analyzer=")
print("This program will analyze a word that is provided by the user and provide some facts about it.")
    
word = input("Please enter your favorite word:\n").upper()

print("\n\nThe total length of your favorite word is: {} characters".format(len(word)))
print("Your favorite word starts with the letter: {} and ends with the letter: {}".format(word[0], word[-1]))

if "S" in word and "N" in word:
    print("Your favorite word contains BOTH letters that make up my initials, S and N.")
else:
    print("Your favorite word does NOT contain BOTH letters that make up my initials, S and N.")
\$\endgroup\$
3
  • \$\begingroup\$ I appreciate the in-depth feedback and commentary. I didn't realize that you could add .upper() to the end of an input request and it would perform the conversion at that time. I like how your were able to shorten the code but still accomplish the same task. \$\endgroup\$
    – c0de4fun90
    Commented Apr 24, 2022 at 15:51
  • 3
    \$\begingroup\$ I should probably mention that there is a corner case where the calling .upper() instead of .lower() changes the behavior: the uppercase form of "ß" is "SS", which changes the number of characters. \$\endgroup\$ Commented Apr 24, 2022 at 16:34
  • \$\begingroup\$ Ah, another useful consideration. Thank you! \$\endgroup\$
    – c0de4fun90
    Commented Apr 24, 2022 at 23:49
1
\$\begingroup\$

Welcome to Code Review, and welcome to the world of Python programming!

Everyone's code is awful at the start; don't worry about that. Just be open to feedback, learn from feedback you get from others. It is OK if you approach a programming task in a different fashion than others; there is more than one way to do it (often abbreviated "TIMTOWTDI"). 200_success gave you great feedback; I'll provide you with some more. You've said you are new to coding, so some of the things I'll be describing will be new to you. Don't worry about that; think of it as a preview of things to come.

Multi-line strings

print("\n")
print("=Word Analyzer=")
print("This program will analyze a word that is provided by the user and provide some facts about it.")

The repetitive print() statements here are superfluous. Consider replacing the above with one print statement using a triple-quoted string:

print("""
=Word Analyzer=
This program will analyze a word that is provided by the user and provide some facts about it.""")

Note that instead of \n, the newline is directly included in the triple-quoted string.

Functions

Use functions to collect statements into a logical function block. Beyond simple organization, this helps created testable code. You can call the function multiple times with different inputs and check to see if the code operates properly in each case.

For instance, the analysis report for the user's word seems like a good place to use a function:

def print_word_analysis(word):
    new_word = word.lower()

    print(f"The total length of your favourite word is: {len(new_word)} characters")

    # ... middle of your code omitted for brevity ...

    if ("s" and "n" in new_word):
        print("Your favorite word contains BOTH letters that make up my initials, S and N.")
    else :
        print("Your favorite word does NOT contain BOTH letters that make up my initials, S and N.")


# Greets user and names program
print("""
=Word Analyzer=
This program will analyze a word that is provided by the user and provide some facts about it.""")

# Queries user for input

favourite_word = input("Please enter your favorite word:\n")

print_word_analysis(favourite_word)

With this function defined, you can try executing a few different tests, with words that contain both of your initials, just one of your initials, or none of them:

print_word_analysis("snow")
print_word_analysis("rain")
print_word_analysis("sleet")
print_word_analysis("dark")

As pointed out be 200_success, your code has a bug. The above tests improperly declare "rain" as containing an S.

Hard Coded Constants

The code presently tests for just your initials. What if I wanted to check for my initials: "AN"? Do I have to change the entire program, searching for all places an "S" is used and replacing it with an "A"? That is complicated by the fact that I'd also have to replace 's' with 'a'.

It would be better if the code defined a constant at the top of the program with the initials being searched for:

INITIALS = "SN"

...
new_word = word.upper()

if INITIALS[0] in new_word and INITIALS[1] in new_word:
    print(f"Your favourite word contains BOTH letters that make up my initials, {INITALS[0]} and {INITIALS[1]}")
else:
    print(f"Your favourite word does NOT contain BOTH letters that make up my initials, {INITIALS[0]} and {INITIALS[1]}.")

Now I can modify the first line to INITIALS = "AN" to analyze words using my own initials.

Except ...

My initials are actually "AJN". If I used INITIALS = "AJN", the code is still hard-coded to only be checking for the first two letters.

We can use a for-else loop instead:

for initial in INITIALS:
    if initial not in new_word:
        print(f"Your favourite word does NOT contain ALL letters that make up my initials: {INITIALS}")
        break
else:
    print(f"Your favourite word contain ALL of letters that make up my initials: {INITIALS}")

The for-else loop will only execute the else: clause if the loop is not exited with the break statement.

Often, these kinds of loops can be converted into an any() or all() test:

if all(initial in new_word for initial in INITIALS):
    print(f"Your favourite word contain ALL of letters that make up my initials: {INITIALS}")
else:
    print(f"Your favourite word does NOT contain ALL letters that make up my initials: {INITIALS}")

Sets

If "Beetle Bailey" ever got around to programming, his initials would be "BB". If the first initial is found in the favourite word, does it even make sense to look for the second initial? Wouldn't looking for B just once make sense?

A set will only ever contain one occurrence of an item. A set of the letters "AN" would be {'A', 'N'} where as a set of the letters "BB" would just be {'B'}.

Python has a whole host of tests you can do with sets. In particular, you can see if a set is a subset of another set. Your test could be replaced be checking if the set of your initials is a subset of the set of all letters in the favourite word.

if set(INITIALS) <= set(new_word):
    print(f"Your favourite word contain ALL of letters that make up my initials: {INITIALS}")
else:
    print(f"Your favourite word does NOT contain ALL letters that make up my initials: {INITIALS}")

Multiple Letters

Does "Bounce" contain all of the initials "BB", or only 1 of the B's? Should it pass, or only words like "Baby" which contain multiple B's?

You can use a Counter to determine the counts of each letter, and compare collections using super/subset relationships:

from collections import Counter

INITIALS = "BB"

...

if Counter(INITIALS) <= Counter(new_word):
    print(f"Your favourite word contain ALL of letters that make up my initials: {INITIALS}")
else:
    print(f"Your favourite word does NOT contain ALL letters that make up my initials: {INITIALS}")

Refactored code

Here is my reworking of your code.

from collections import Counter

INITIALS = "BB"

def print_word_analysis(word: str, required_letters: str) -> None:
    """
    Analyze the given word, reporting on length, first and last letter,
    and whether or not all of the required letters are present.

    >>> print_word_analysis("snow", "SN")
    The total length of your favorite word is 4 characters
    Your word starts with the letter S and ends with the letter W
    Your word contains ALL of the required letters: SN

    >>> print_word_analysis("rain", "sn")
    The total length of your favorite word is 4 characters
    Your word starts with the letter R and ends with the letter N
    Your word does NOT contain ALL of the required letters: SN
    """
    
    print(f"The total length of your favorite word is {len(word)} characters")

    word = word.upper()

    print(f"Your word starts with the letter {word[0]}"
          f" and ends with the letter {word[-1]}")

    required_letters = required_letters.upper()

    if Counter(required_letters) <= Counter(word):
        print(f"Your word contains ALL of the required letters: {required_letters}")
    else :
        print(f"Your word does NOT contain ALL of the required letters: {required_letters}")

if __name__ == '__main__':
    import doctest
    doctest.testmod()
    
    print("""
=Word Analyzer=
This program will analyze a word that is provided by the user and provide some facts about it.""")

    word = input("Please enter your favorite word:\n")

    print_word_analysis(word, INITIALS)

I've added in a few more things. The INITIALS are now passed to the report function as required_letters. The report function has type hints for the parameters and return value, as well as a """docstring""" describing the function.

Embedded in the """docstring""" are two tests that demonstrate how the function works, and in the __name__ == '__main__' guard, the doctest module is imported, and the tests are (quietly) run. Try modifying the function or the tests slightly and rerunning the code to see what happens when the doctests fail.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I really appreciate the feedback and detail you have provided here. This is alot of great information! \$\endgroup\$
    – c0de4fun90
    Commented Apr 26, 2022 at 2:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.