#Code Optimization request

1 messages · Page 1 of 1 (latest)

astral smeltBOT
#

@storm drift

samseito02 Uploaded Some Code

Hi there !
So in this code, I would like to create a form of decision support by implementing a questionnaire. Based on 'yes' or 'no' responses, I allocate points to various models (in this case, smartphone models). However, I'm facing repetition as similar code segments are used for multiple questions. Is there a way to achieve the same outcome in a more 'concise' manner?
PS: I've assigned points randomly, it's just for testing purposes

Uploaded these files to a Gist

phone.py

View The Gist

For safety reasons we do not allow file attachments.
remote frost
#

Ok, buckle up haha

    q1_model = []
    q1_text = "Is the price important to you? "
    q1_model_yes = [models[3], models[2]]
    q1_model_no = [models[4], models[6]]

    q1_model = q1_model_yes + q1_model_no

    question_01 = Question(q1_text, q1_model)
    questions.append(question_01)

That's unnecarilly complex.

    questions.append(
        Question(
            "Is the price important to you? ",
            [models[3], models[2], models[4], models[6]]
        )
    )

Much simpler... I'm also unsure why you're do yes and no lists and then adding them together... That just creates a single list, not two lists.

I'd also suggest using yield instead of appending to a list that you're returning. Makes the code simpler, more efficient, and more flexible.

def question_creation(models):
    yield Question(
        "Is the price important to you? ",
        [models[3], models[2], models[4], models[6]]
    )

    yield Question(
        "Is a good cammera a primary option for you? ",
        [models[0], models[4], models[5], models[3],models[6]]
    )
    
    ...
#

Here there's no need for a new variable, also only use i for indexes.

    questions = question_creation(models)
    for i in questions:
        i.ask()

change it to

    for question in question_creation(models):
        question.ask()
#

You almost never should use an infinite loop.

    def ask(self):
        while True:
            answer = input(f"{self.text} (Yes/No): ").strip().lower()
            if valid_answer(answer):
                for model in self.attribution:
                    model.score += 1
                break
            else:
                print('Palease, answer by "Yes" or "No"')

should be (broke it up into multiple functions to clean it up a bit)

def get_input(message):
    return input(message).strip().lower()


def get_valid_answer(message):
    answer = get_input(message)
    while not valid_answer(answer):
        print('Please, answer "Yes" or "No"')
        answer = get_input(message)

    return answer


def ask(self):
    answer = get_valid_answer(f"{self.text} (Yes/No): ")
    for model in self.attribution:
        model.score += 1
#

You've also got a logic error in ask since it adds points to each model for both "yes" and "no"

storm drift
#

wow, that's a lot indeed, thank you !
I now see all the mistakes, and for the yes or no, i forgot to add something, i wanted to do like:
if answer == "yes" :
the "q_model_yes" gonna have points
else :
the "q_model_no" one

that's why i did two lists, but i'm not sure that this will work

remote frost
#

You'll need to have yes and no lists on the Question class

storm drift
#

ah yes, that's clear now
thank you !
i'll fix that

astral smeltBOT
#

@storm drift

samseito02 Uploaded Some Code

ok i tried to fix it
is it better now ?

Uploaded these files to a Gist

new_phone.py

View The Gist

For safety reasons we do not allow file attachments.
storm drift
#

is it now better ?

little vector
#

Looks good, I'd probably change the following:

  1. Add type hints (I find those very useful, they also improve IDE suggestions and make the code more clear in general
  2. I think there's a bug in the get_valid_answer function, you never actually return the answer (so you should probably return it after the while loop)
  3. Rewrite the loops in the ask function like this:
targets = self.model_yes if answer == "yes" else self.model_no
for model in targets:
    model.score += 1

Using a dictionary to store the different models might be even easier, that just comes down to preference in the end though