4
\$\begingroup\$

I have commented out two attributes (in random_number_guesser.py) which I believe are unnecessary.

Am I correct in assuming that, because the code runs correctly without their implementation, they should not be included in my program?

player.py

class Player:

    def __init__(self) -> None:
        """Initializes the player object's attributes."""
        self.wins = 0

    def create_username(self) -> None:
        """Prompts the player to enter their username."""
        attempts = 1
        MAX_ATTEMPTS = 3

        while attempts <= MAX_ATTEMPTS:

            self.username = input("What is your name?: ")

            # Checks if the username contains only whitespace.
            if not self.username.strip():
                if attempts == MAX_ATTEMPTS:
                    self.username = "Player_1"
                else:
                    print()
                    print("Your username must contain at least one character.")
            else:
                self.username = self.username.strip()
                break
            
            attempts += 1

    def __str__(self) -> str:
        """Returns the player's username as a string."""
        return self.username
    
    def get_guess(self) -> int:
        """Returns the player's guess as an integer."""

        while True:
            
            try:
                self.guess = int(input("Can you guess what it is?: "))
                return self.guess
            
            except ValueError:
                print()
                print("Please enter a whole number.")

    def add_win(self) -> None:
        """Adds a win to the player's win count."""
        self.wins += 1

random_number_guesser.py

from player import Player
import random

class RandomNumberGuesser:

    def __init__(self) -> None:
        """Initializes the game object's attributes."""
        #self.player: Player | None = None
        #self.winning_number: int | None = None

    def create_player(self) -> None:
        """Creates a new player object."""
        self.player = Player()
        self.player.create_username()

    def greet_player(self) -> None:
        """Greets the player."""
        print(f"Welcome, {self.player}!")

    def generate_random_number(self) -> None:
        """Generates a winning number."""
        MIN = 1
        MAX = 10

        self.winning_number = random.randint(MIN, MAX)
        print()
        
        #DEBUG - Displays the winning number:
        #print(f"The winning number is: {self.winning_number}.")

        print("I'm thinking of a number between 1 & 10.")
        
    def guess_and_check(self) -> None:
        """Compares the player's guess to the winning number."""
        
        # Checks if the player's guess is correct.
        if self.player.get_guess() == self.winning_number:
            print()
            print(f"{self.player} Won!")
            self.player.add_win()
        else:
            print()
            print(f"{self.player} Lost!")

    def play_again(self) -> None:
        """Determines whether or not the player wants to play again."""

        while True:

            play_again = input("Would you like to play again? (y/n): ")

            # Checks if the player wants to play again & handles invalid inputs.
            if play_again == "y":
                self.generate_random_number()
                self.guess_and_check()

            elif play_again == "n":
                print()
                
                # Checks the player's win count and determines if 'time' or 'times' should be used.
                if self.player.wins == 1:
                    print(f"{self.player} won {self.player.wins} time!")
                else:
                    print(f"{self.player} won {self.player.wins} times!")
                break

            else:
                print()
                print("Please enter either 'y' or 'n'.")

def play_random_number_guesser() -> None:
    print("Welcome To Random Number Guesser!")
    print("=================================")

    game = RandomNumberGuesser()
    game.create_player()
    game.greet_player()
    game.generate_random_number()
    game.guess_and_check()
    game.play_again()

game.py

from random_number_guesser import *

if __name__ == "__main__":
    play_random_number_guesser()
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Welcome to Code Review! To help reviewers give you better answers, we need to know what the code is intended to achieve. Please add sufficient context to your question to describe the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. Also, edit the title to simply summarise the task, rather than your concerns about the code. \$\endgroup\$ Nov 5 at 13:18
  • \$\begingroup\$ Are you the author and/or maintainer of the code? \$\endgroup\$ Nov 5 at 14:45
  • 1
    \$\begingroup\$ Yes, I am the author of the code. \$\endgroup\$ Nov 5 at 15:24
  • \$\begingroup\$ That's... debatable. The code started off as yours, but this version is mostly mine (minus the commented attributes). \$\endgroup\$
    – Reinderien
    Nov 5 at 20:55
  • 1
    \$\begingroup\$ It's totally fine - I only brought it up because Sam asked. I won't make the judgement call as to whether it's off-topic (I'll leave that to the mods), but generally, asking how a certain thing works when you haven't written that thing is not supported on CodeReview. \$\endgroup\$
    – Reinderien
    Nov 5 at 21:56

2 Answers 2

10
\$\begingroup\$

Am I correct [that] ... they are unnecessary to include in my program?

Good question.

Incorrect. You should continue to include them.

class RandomNumberGuesser:

    def __init__(self) -> None:
        """Initializes the game object's attributes."""
        #self.player: Player | None = None
        #self.winning_number: int | None = None

    def create_player(self) -> None: ...
        self.player = Player() ...        

    def generate_random_number(self) -> None:
        ...
        self.winning_number = random.randint(MIN, MAX)

Now, I see where you're coming from, it is totally sensible that you might conclude "bah, useless!" and rip it out. The machine doesn't care. But people do.

programs must be written for people to read, and only incidentally for machines to execute.

-- SICP

In many OO languages an object constructor is responsible for declaring / allocating storage for all attributes which the object might use over its lifetime. Python is a dynamic language where strictly speaking this is not necessary -- attributes can be tacked onto an object, and even destroyed, any time the object is accessible. But declarations are helpful to humans. Good manners dictates that we conventionally introduce every attribute we will ever use, right up front in the constructor. If we do not yet have some sensible value we just assign None and move on.

This is a kindness to the Gentle Reader. Seeing each attribute reminds us to perhaps describe it in the __init__ constructor """docstring""", or to write a # comment for it. Each class is responsible for maintaining its class invariants over the attributes, and that responsibility starts the moment the constructor returns. They can be weak or strong invariants. The stronger the guarantees, usually the easier it is for a maintenance engineer to reason about object behavior.

So yes, please retain that "catalog" of attributes, even if we're not yet ready to fill them in at object init time.


Here is a possible refactor you might consider:

    MIN = 1
    MAX = 10

    def __init__(self) -> None:
        """Initializes the game object's attributes."""
        self.player: Player = self._create_player()
        self.winning_number: int = random.randint(self.MIN, self.MAX)

    def _create_player(self) -> Player:
        """Creates a new player object."""
        player = Player()
        player.create_username()
        return player

Notice that we're making a stronger guarantee. Can self.player be "None or a Player" ? No, now it can only be a Player. And similarly we promise to always have an int.

Now a maintenance engineer has one less case to consider. No need to worry about potential None, as the class promises that that won't happen. And we can use mypy to verify it stays that way, even after weeks of code edits.

\$\endgroup\$
7
  • 8
    \$\begingroup\$ All correct. It's worth mentioning that this isn't just something you made up; open-source tools like pylint throw W0201 when encountering non-__init__ members. \$\endgroup\$
    – Reinderien
    Nov 4 at 19:28
  • \$\begingroup\$ Thanks for the clarification! \$\endgroup\$ Nov 4 at 23:13
  • 2
    \$\begingroup\$ Better yet, why should RandomNumberGuesser know how to create a player? Have __init__ take an instance of Player as a required argument instead. \$\endgroup\$
    – chepner
    Nov 5 at 21:37
  • 1
    \$\begingroup\$ In modern CPython, the "machine" does care. The key-sharing dict optimization used by CPython to reduce per-instance memory usage relies on the attributes being unconditionally initialized, in the same order, for all instances (which is not possible to guarantee unless __init__ is uniformly initializing them all). So it's not just for humans, it also makes any class with many instances use substantially less memory per-instance. \$\endgroup\$ Nov 5 at 21:45
  • \$\begingroup\$ @ShadowRanger, wow, thank you! I get the "key sharing" notion. But, benchmarks? Please! I imagine someone has gone to the trouble of showing how memory footprint is dramatically lower if a class plays by the rules? \$\endgroup\$
    – J_H
    Nov 6 at 3:23
3
\$\begingroup\$

Unnecessary to the execution of the specific task the code performs is not the same as unnecessary to the proper usage of the code nor is it the same as irrelevant to good maintenance of the code, nor the same as irrelevant to ease in evolving the code.

Code has context and a purpose that extends beyond the algorithm and its output.

Sometimes it doesn’t extend much beyond that, because it’s a onetime effort to get a specific result, that is itself just being used for a one time effort.

Sometimes it is part of an evolving and essential process.

Whether or not attributes, comments and names are important are a function of context and purpose.

Given the code and names in it, this is probably either schoolwork or hobby exploration. If hobby exploration, then it’s up to you and your goals as to what is unnecessary and what is not. If schoolwork, it depends on grading and how you value that grading.

TL;DR: If this is part of a long term collaboration such as a company project or scientific research, it adds useful information for your fellows.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the response! I now realize that we should write code that is functional and easy to follow. I would like to clarify that this is not school work. I do not use Chat GPT to generate any code, nor do I copy straight from tutorials. I feel that doing so would defeat the purpose of trying to actually learn the language. \$\endgroup\$ Nov 5 at 15:48
  • 1
    \$\begingroup\$ @dylanbobbyromero1: no problem with hobby exploration, my point is that with a hobby exploration, you get to set the goals of your project. It’s like doing a code kata vs code golf. \$\endgroup\$
    – jmoreno
    Nov 5 at 19:25

Your Answer

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

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