I was reviewing the “first posts” queue on StackOverflow yesterday and one of questions I had to review was not particularly clear nor very well written.
Basically, the poster had a script which was looping and interacting with the user and her question was (I think) how to exit this infinite loop at some point.
The question was unclear, and the code wasn’t particularly elegant, not really what can be regarded as a Minimal Working Example, yet it was actually running. Needless to say, by the time I was ready to make some comments and edits, the post had already been closed as “off-topic”.
In my mind, that’s a bit of a shame. I understand it wasn’t the best question in the world but, picture this: there’s someone who’s trying to write some program, she manages to cobble together some code (we all started out like that, come on), she actually makes it run, stumble upon a blocker, turns to the wisdom of the (geek) crowd for help… only to get told to ask a better question. Like, “Yeah yeah, sorry, got no time, can’t understand what you want, go and play hide and seek on the Autobahn”.
I don’t know, it feels wrong. I mean, people will have to start off at some point, they can make mistakes, right? Which is why I flagged the post and asked it to move it to the Code Review StackExchange — awesome for this particular type of questions.
Got no response, most likely my request was ignored, no big deal. Anyhow, I edited the post today and asked (again) for it to be moved to CR. I’m not sure it’ll ever be re-opened and moved, so what I thought of doing is give my view of that code here on my blog and offering solution. It can (of course) still be improved, but it’s a start.
Data structures
#Phones
p = ["BPCM", "BPSH", "RPSS", "RPLL", "YPLL", "YPLL"]
p_desc = ["Compact", "Clam Shell", "RoboPhone - 5-inch screen and 64 GB memory", "RoboPhone - 6-inch screen and 256 GB memory", "Y-Phone Standard – 6-inch screen and 64GB memory" , "Y-Phone Deluxe – 6-inch screen and 256GB memory", ""]
p_price = [29.99, 49.99, 199.99, 499.99, 549.99, 649.99]
#Tablets
t = ["RTMS", "RTLM", "YTLM", "YTLL"]
t_desc = ["RoboTab – 8-inch screen and 64GB memory", "RoboTab – 10-inch screen and 128GB memory", "Y-tab Standard - 10-inch screen and 128GB memory", "Y-tab Deluxe - 10-inch screen and 256GB memory"]
t_price = [149.99, 299.99, 499.99, 599.99]
The snippet above shows something interesting. for phones you have a list of product codes, followed by a list of descriptions, followed by a list of prices. The poster commented she wasn’t able to use 2-D array, presumably because she wanted to have a list of lists, the latter being something like “code, description, price”.
Since the structure is followed by all the items in the code, a better solution would be to collect these properties into an object.
class Item:
"""Class representing a generic item, having a code, description and a price"""
def __init__(self, code, description, price):
self.code = code
self.description = description
self.price = price
def __str__(self):
return "{} ({}) - price: {}".format(self.description, self.code, self.price)
def __repr__(self):
return "Item({}, {}, {})".format(self.code, self.description, self.price)
So that you can define a phone or a tablet as Item("BPCM", "Compact", Decimal("29.99"))
An additional improvement we can do is the use of something better than lists for collecting our items. I’d suggest a simplified mapping where items are stored by code:
class ItemCollection:
"""Collection of Items, stored by code"""
def __init__(self, *items):
self.collection = {item.code: item for item in items}
def __getitem__(self, code):
return self.collection[code]
def __iter__(self):
return iter(self.collection.items())
def codes(self):
return self.collection.keys()
Flow control
running = True
while running == True:
print ("Do you want a phone or a tablet")
Couple of things we can do better here. First, checking wether a value is True (or False) should be done through the identity operator (is) not the equality one (==). This is because there is only one True object. Second, we can get rid of this check altogether, as we can simply break out of the loop at some point, depending on what the user of the script decides.
This would become something like:
while True:
...
add_another = present_choices("Add another device? ", {"Y", "N"}).upper()
if add_another != "Y":
break
Input/Output
if phone_tablet == ("phone"):
print("\nCode -")
print (p)
print("\nDescription -")
print(p_desc)
print("\nprice -")
print(p_price, "\n")
p_choice = input ("Enter the code of the phone \n")
while p_choice not in p:
print("Unvalid code, try again")
p_choice = input("Enter the code again ")
pass
Lots of prints going on here. We can probably tidy them up. Also there’s a superfluous pass statement that can be removed and some typos (Unvalid). What we can notice is also that the prints are repeated in a similar fashion throughout the code, though they’re not exactly the same. We could probably reuse them and create a function. Something like:
def show_items(items, intro_text=None, outro_text=None):
"""Print to the standard output the items in the collection
:param items: ItemCollection instance
:param intro_text: (optional) text to prepend to the output
:param outro_text: (optional) text to append to the output
"""
line_format = "{:<4} | {:<50} | {:<8}"
header = line_format.format("Code", "Description", "Cost")
separator = "-" * len(header)
if intro_text:
print(intro_text)
print()
print()
print(header)
print(separator)
for _, item in items:
print(line_format.format(item.code, item.description, item.price))
print(separator)
if outro_text:
print(outro_text)
print()
It could be made even more generic (notice that it has knowledge of “Code”, “Description” and “Cost”) and, in general, better (fewer print()s) but it’s a valid starting point.
Getting the input is also pretty much the same operation, regardless of the type of the device (enter something, if not valid try again, otherwise continue). Here, too, we can reuse our code if we put it into a function.
def present_choices(prompt, valid_choices, error_prompt="Invalid choice. ") -> str:
"""present_choices: ask the user for input
:param prompt: string to present to the user (e.g. "Type 'phone' or 'tablet': ")
:param valid_choices: collection of inputs that can be accepted
:param error_prompt: string to present to the user in case she fails to give a valid choice,
defaults to "Invalid choice. ".
:return: string representing the choice made
"""
choice = input(prompt)
while choice not in valid_choices:
choice = input(f"{error_prompt}{prompt}")
return choice
General comments
Other things that can be improved in general:
- Respect PEP 8 for better maintainability – it always pays off.
- Write unit tests to fend off regressions – no quibbling!
- Type hints could also help. I’m guilty myself here, coming from a background almost exclusively made of Py2.
- Encoding directive at the beginning of the script. This is another thing that caught me off: the original code contains some non-ASCII characters and Python will complain when trying to parse it unless you add something like
-*- coding: utf-8 -*- at the top of the script. - The use of f-strings, reducing the amount of code needed to populate strings.
- Anything else I missed? Anything that’s not quite right? Let me know in a command: I’ll be happy to know (how would anyone learn without feedback?!)
My attempt at a solution would look like this:
#!/usr/bin/env python3
# -*- coding: utf-8 -*-
from decimal import Decimal
class Item:
"""Class representing a generic item, having a code, description and a price"""
def __init__(self, code, description, price):
self.code = code
self.description = description
self.price = price
def __str__(self):
return "{} ({}) - price: {}".format(self.description, self.code, self.price)
def __repr__(self):
return "Item({}, {}, {})".format(self.code, self.description, self.price)
class ItemCollection:
"""Collection of Items, stored by code"""
def __init__(self, *items):
self.collection = {item.code: item for item in items}
def __getitem__(self, code):
return self.collection[code]
def __iter__(self):
return iter(self.collection.items())
def codes(self):
return self.collection.keys()
phones = ItemCollection(
Item("BPCM", "Compact", Decimal("29.99")),
Item("BPSH", "Clam Shell", Decimal("49.99")),
Item("RPSS", "RoboPhone - 5-inch screen and 64 GB memory", Decimal("199.99")),
Item("RPLL", "RoboPhone - 6-inch screen and 256 GB memory", Decimal("499.99")),
Item("YPLL", "Y-Phone Standard – 6-inch screen and 64GB memory", Decimal("549.99")),
Item("YPLL", "Y-Phone Deluxe – 6-inch screen and 256GB memory", Decimal("649.99")))
tablets = ItemCollection(
Item("RTMS", "RoboTab – 8-inch screen and 64GB memory", Decimal("149.99")),
Item("RTLM", "RoboTab – 10-inch screen and 128GB memory", Decimal("299.99")),
Item("YTLM", "Y-tab Standard - 10-inch screen and 128GB memory", Decimal("499.99")),
Item("YTLL", "Y-tab Deluxe - 10-inch screen and 256GB memory", Decimal("599.99")))
sim_cards = ItemCollection(
Item("SMNO", "SIM Free (no SIM card purchased)", Decimal("0.00")),
Item("SMPG", "Pay As You Go (SIM card purchased)", Decimal("9.99")))
cases = ItemCollection(
Item("CSST", "Standard", Decimal("0.00")),
Item("CSLX", "Luxury", Decimal("50.00")))
chargers = ItemCollection(
Item("CGCR", "Car", Decimal("19.99")),
Item("CGHM", "Home", Decimal("15.99")),
Item("CGNO", "No charger", Decimal("0.00")))
total_price = Decimal("0")
def show_items(items, intro_text=None, outro_text=None):
"""Print to the standard output the items in the collection
:param items: ItemCollection instance
:param intro_text: (optional) text to prepend to the output
:param outro_text: (optional) text to append to the output
"""
line_format = "{:<4} | {:<50} | {:<8}"
header = line_format.format("Code", "Description", "Cost")
separator = "-" * len(header)
if intro_text:
print(intro_text)
print()
print()
print(header)
print(separator)
for _, item in items:
print(line_format.format(item.code, item.description, item.price))
print(separator)
if outro_text:
print(outro_text)
print()
def present_choices(prompt, valid_choices, error_prompt="Invalid choice. ") -> str:
"""present_choices: ask the user for input
:param prompt: string to present to the user (e.g. "Type 'phone' or 'tablet': ")
:param valid_choices: collection of inputs that can be accepted
:param error_prompt: string to present to the user in case she fails to give a valid choice,
defaults to "Invalid choice. ".
:return: string representing the choice made
"""
choice = input(prompt)
while choice not in valid_choices:
choice = input(f"{error_prompt}{prompt}")
return choice
while True:
device_type = present_choices("Which type of device would you like? Type 'phone' or 'tablet': ",
{"phone", "tablet"})
devices = phones if device_type == "phone" else tablets
show_items(devices)
device_code = present_choices("Enter the code of the device of your choice: ", devices.codes())
show_items(sim_cards)
sim_card_code = present_choices("Enter the code of the SIM card of your choice: ", sim_cards.codes())
show_items(cases)
case_code = present_choices("Enter the code of the case of your choice: ", cases.codes())
show_items(chargers)
charger_code = present_choices("Enter the code of the case of your choice: ", chargers.codes())
selection = ItemCollection(
devices[device_code],
sim_cards[sim_card_code],
cases[case_code],
chargers[charger_code])
price = sum(i.price for (_, i) in selection)
total_price += price
show_items(selection, intro_text="Summary of your selection", outro_text=f"The total price is {price}")
add_another = present_choices("Add another device? ", {"Y", "N"}).upper()
if add_another != "Y":
break
print(f"The total price is {total_price}")
Who knows, maybe the poster will never stumble upon this blog post.
Or maybe she will, in which case I’d tell her “Hang on, you’ll soon be on the other side of the fence, doing code reviews for those who have just started!”.
Mind the test 
