exercise#1
Conversation
|
Impressive work and cool functionality! The ability to refresh and select a specific character is pretty cool, along with the functionality of using the API to match by the first few characters! |
|
Two things before diving into the code:
|
PhilipDW183
left a comment
PhilipDW183
left a comment
There was a problem hiding this comment.
Very clear variable naming throughout! It was pretty easy to read and understand what each variable was doing.
However, a lot of the code could have been put into functions. This would have made it easier to understand what the flow is meant to be and made it easier to read and understand. It would also have ensured that if your code was imported somewhere else, it wouldn't just run.
| PUBLIC_API_KEY = getenv("MARVEL_PUBLIC_API_KEY") | ||
| PRIVATE_API_KEY = getenv("MARVEL_PRIVATE_API_KEY") |
There was a problem hiding this comment.
Nice use of keeping your environment variables secure! Good DevOps practice!
| SEARCH_URL = "https://gateway.marvel.com:443/v1/public/characters" | ||
| DETAILS_URL = "https://gateway.marvel.com:443/v1/public/characters/{id}" |
There was a problem hiding this comment.
Is there a way you could have just added the {id} onto the first url rather than having to create two urls?
| from time import time_ns | ||
|
|
||
| from requests import get |
There was a problem hiding this comment.
Why the gap here? It would be good to keep all imports together unless there is some specific reason not to
| while True: | ||
| timestamp = str(time_ns()) | ||
| key_hash = md5(f"{timestamp}{PRIVATE_API_KEY}{PUBLIC_API_KEY}".encode("utf-8")).hexdigest() | ||
| default_params = { | ||
| 'apikey': PUBLIC_API_KEY, | ||
| 'ts': timestamp, | ||
| 'hash': key_hash, | ||
| } | ||
|
|
||
| name = input("What Marvel character? ") | ||
|
|
||
| keep_searching = True | ||
| ID = None | ||
| offset = 0 | ||
|
|
||
| while keep_searching: |
There was a problem hiding this comment.
A nested while statement could be suggested to not be best practice as it can get messy (see comment below on nested if/else statements)
| data = res['data'] | ||
| total = data['total'] | ||
| offset = data['offset'] | ||
|
|
||
| characters = data['results'] |
There was a problem hiding this comment.
Have you thought about using .get() instead of []. This would just ensure that an error wouldn't be thrown if that specific section wasn't there.
| print(f"- {i}: {character['name']}") | ||
| IDs.append(character['id']) | ||
|
|
||
| choice = input(f"Which ID? (1-{len(characters)}), or 'n' for next page ") |
There was a problem hiding this comment.
This is pretty cool functionality! Awesome job!
| if total == 0: | ||
| print("Couldn't find any characters by this name...") | ||
| keep_searching = False | ||
| elif total == 1: | ||
| ID = characters[0]['id'] | ||
| keep_searching = False | ||
| else: | ||
| IDs = [] | ||
| i = 0 | ||
| print(f"Please select a character)") | ||
| for character in characters: | ||
| i += 1 | ||
| print(f"- {i}: {character['name']}") | ||
| IDs.append(character['id']) | ||
|
|
||
| choice = input(f"Which ID? (1-{len(characters)}), or 'n' for next page ") | ||
|
|
||
| if choice == "n": | ||
| if offset + PAGE_SIZE >= total: | ||
| print("There are no more pages...") | ||
| keep_searching = False | ||
| else: | ||
| offset += PAGE_SIZE | ||
| else: | ||
| index = int(choice) - 1 | ||
| ID = IDs[index] | ||
| keep_searching = False | ||
| found_a_character = True |
There was a problem hiding this comment.
At this point there is many nested if/else if statements, on top of the nested while statements, this makes following the logic difficult, especially to understand which branch you end up in. If there are nested if/else statements, then sometimes it is best to put everything into a function and use return statements.
| print(f"Name: {character['name']}") | ||
| print(f"Bio: {character['description']}") |
There was a problem hiding this comment.
Nice functionality to print both the name and the description!
2 participants
No description provided.