EagleSury / GuessTheCountry

0 stars 0 forks source link

Feedback on Code: #1

Open lenisha opened 1 month ago

lenisha commented 1 month ago

@EagleSury Couple of thoughts on code:

Couple things to improve

EagleSury commented 1 month ago

Is my grade final - or can I still try to change things?

If I can , I have a couple of questions: documentation about the app - in general? Like what the app does and how it does it? Where should I be writing it? requirements.txt is supposed to be just a list? Is there a way I can check that I included everything?

On Fri, Aug 2, 2024 at 8:23 PM Elena Neroslavskaya @.***> wrote:

@EagleSury https://github.com/EagleSury Couple of thoughts on code:

  • Good work on making WebScaping, SQLLite and Flask application work!!

Couple things to improve

  • A bit more details in documentation about the app and requirements.txt file missing on what to install to make it run
  • Connection handling and closing when finished logic needs to happen. For example function get_length_of_data has connection leak
  • There is generally no need for session object as the whole interaction could be stateless, and data just passed to html page
  • A bit convoluted logic in main function set_new_variables double loops are not the most optimal. To make unique choices better use Set as a data structure, for example:

def set_new_variables(): length_of_data = get_length_of_data() choice = random.randint(1, length_of_data) # assuming primary key starts at 1 chosen_object = Flag.query.get(choice)

session['flag'] = chosen_object.flag
session['correct_country'] = chosen_object.country

choices = {session['correct_country']}
while len(choices) < 5:
    new_index = random.randint(1, length_of_data)
    new_country = Flag.query.get(new_index).country
    choices.add(new_country)

session['choices'] = list(choices)
  • Tests not fully functional - could have tested generation of choices, picking wrong or correct answers etc

— Reply to this email directly, view it on GitHub https://github.com/EagleSury/GuessTheCountry/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFFOBCURSYPVW6MAUGUFC3TZPO6B3AVCNFSM6AAAAABL45S3DGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ2DKNBUHA4DSMI . You are receiving this because you were mentioned.Message ID: @.***>