This is a follow up question to PyDOS shell simulation.
You might remember PyDOS from my other post. I have now updated it and it's better than ever! Post any ideas below that could make it better.
import time
import os
import sys
import random
def splash():
print ()
print ("PyDOS")
print ()
print ("Version 1.9.8")
print ()
time.sleep(3.2)
def calc():
while True:
print ("Welcome to PyCALC")
print ("Please type the number for your suggestion.")
print ("1: Addition\n2: Subtraction\n3: Multiplacation\n4: Division")
suggestion = input("Your Choice: ")
if suggestion == "1":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1+num2
print ("Your answer is:")
print (answer)
break
if suggestion == "2":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1-num2
print ("Your answer is:")
print (answer)
break
if suggestion == "3":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1*num2
print ("Your answer is:")
print (answer)
break
if suggestion == "4":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1/num2
print ("Your answer is:")
print (answer)
break
else:
print ("Your operation choice is invalid!")
def get_lines():
print("Enter 'stop' to end.")
lines = []
line = input()
while line != 'stop':
lines.append(line)
line = input()
return lines
def textviewer():
os.system('cls' if os.name == 'nt' else 'clear')
print ("Text Viewer.")
directory = ("/media/GENERAL/Projects/files")
filename = input("Enter a text file to view: ")
file = open(os.path.join(directory, filename), "r")
print ("Loading text...")
time.sleep(0.5)
os.system('cls' if os.name == 'nt' else 'clear')
print(file.read())
edit_text = input("Would you like to edit it? (y for yes, n for no)")
if edit_text == "y":
file = open(os.path.join(directory, filename), "w")
print ("You are now in edit mode.")
lines = get_lines
file.write('\n'.join(lines))
time.sleep(2)
if edit_text == "n":
print ("Press enter to exit")
input()
def edit():
os.system('cls' if os.name == 'nt' else 'clear')
print ("EDIT")
print ("-------------")
print ("Note: Naming this current document the same as a different document will replace the other document with this one.")
directory = ("/media/GENERAL/Projects/files")
filename = input("Plese enter a file name.")
file = open(os.path.join(directory, filename), "w")
print ("FILE: " + filename+".")
lines = get_lines()
file.write('\n'.join(lines))
def cls():
os.system('cls' if os.name == 'nt' else 'clear')
splash()
while True:
os.system('cls' if os.name == 'nt' else 'clear')
print ()
print ("PyDOS VERSION 1.9.5")
shell = input("> ")
if shell == "textviewer":
print ("Loading Text Viewer...")
time.sleep(3)
textviewer()
elif shell == "help":
print ("HELP")
print ("-----------------")
print ("Commands:")
print ("dir - displays directory.")
print ("cls - clears screen")
print ("help - shows help")
print ("textviewer - launches textviewer")
print ("edit - launches edit")
print ("news - launches news")
print ("shutdown - closes PyDOS")
print ("calc - launches calculator")
print ("------------------------------------")
print ("What is PyDOS?")
print ("PyDOS is inspired by MS-DOS made in the 1980's and")
print ("has that feel of it too! For a better experiance")
print ("run PyDOS in the python CMD shell.")
input ("Press enter to close.")
elif shell == "edit":
print ("Loading edit...")
time.sleep(3)
edit()
elif shell == "calc":
calc()
elif shell == "dir":
print ("The drive name is A:")
print ()
print ("NAME: TYPE: MODIFIED:")
print ("SHUTDOWN.EXE .EXE 12/01/15 ")
print ("EDIT.EXE .EXE 09/03/15 ")
print ("TEXTVIEWER.EXE .EXE 09/03/15 ")
print ("NEWS.EXE .EXE 09/01/15 ")
print ("HELP.COM .COM 09/03/15 ")
print ("HANGMAN(BROKEN).EXE .EXE 11/03/15 ")
print ("CALC.EXE .EXE 20/03/15 ")
input ("---------------Press enter to close---------------")
elif shell == "cls":
cls()
elif shell == "hangman":
hangman.main()
elif shell == "news":
print ("PyDOS NEWS")
print ()
print ("New Additions:")
print ()
print ("New calculator app!")
print ("All text documents from edit are now stored in a 'files' directory")
print ()
print ("Tweaks and fixes:")
print ()
print ("BUG023L: Fixed issue with splash screen loop")
print ()
print ("Reported Bugs:")
print ("BUG024T: Hangman returns a traceback error, we are fixing this!")
print ("BUG025T: Pressing 'y' in texteditor when it asks you if you want\nto edit the file returns Type_Error")
input("Press enter to close")
elif shell == 'shutdown':
print ("Shutting down...")
time.sleep(3)
break
else:
print("This command or file: "+shell+ " does not exist. Check for spelling errors and try again.\nIf you are trying to open a textfile, open textviewer.")
time.sleep(5)
os.system('cls' if os.name == 'nt' else 'clear')
4 Answers 4
Beautiful is better than ugly
Your calculator is ugly. @Hosch gave some hints but a much more dramatic redesign is needed. Why must the user say a number that is mapped to a symbol that is mapped to an operation only then applied to the numbers? This is user unfriendliness taken to the extreme. I suggest this very primitive but much more sane implementation:
def calc():
print ("Welcome to PyCALC")
while True:
expr = input("> ")
a,oper,b = expr.split()
operations = {'+': op.add,
'-': op.sub,
'*': op.mul,
'/': op.truediv
}
print(operations[oper](int(a),int(b)))
Now the user interface is:
> 2 + 2
4
Readability counts
To a poor average human this is just noise:
os.system('cls' if os.name == 'nt' else 'clear')
You have been told to make a function out of this.
Ironically enough, you defined a (badly named) function cls
but you did not use it ?!
First class bug
lines = get_lines
get_lines
is a function... you are saving the content of a function in the text file... calling a function is done by applying double parenthesis get_lines()
Be lazy
The good programmer is lazy, he/she never does by hand what could be automated:
print ("dir - displays directory.")
print ("cls - clears screen")
print ("help - shows help")
print ("textviewer - launches textviewer")
print ("edit - launches edit")
print ("news - launches news")
print ("shutdown - closes PyDOS")
print ("calc - launches calculator")
Let's say you add another utility or change the functionality of one, will you remember to update this list? Maybe.
This list can be generated by using a dictionary but I will not give you the fish.
"Please use {}".format(".format()")
print("This command or file: "+shell+ " does not exist. Check for spelling errors and try again.\nIf you are trying to open a textfile, open textviewer.")
Using {}.format
inside strings is common practice, do not use +
CONSTANT
ly improving maintainability
You may want to change some of your longer strings, looking for them inside the code is not the best option. Instead define them as constants at the start of your file:
NEWS = """"PyDOS NEWS"
"New Additions:"
"New calculator app!"
"All text documents from edit are now stored in a 'files' directory"
"Tweaks and fixes:"
"BUG023L: Fixed issue with splash screen loop"
"Reported Bugs:"
"BUG024T: Hangman returns a traceback error, we are fixing
"BUG025T: Pressing 'y' in texteditor when it asks you if you want\nto edit the file returns Type_Error"
"""
you will then
print(NEWS)
The master directory
"/media/GENERAL/Projects/files"
Why this directory? Why can't the user change this? He/she may want to change the location where the files are saved.
You are still sleeping
I said this, others said this, but I will repeat it:
sleeping does not make the terminal feel retro, it is seriously annoying!
Dealing with files with with
Repetita iuvant // repeating helps
with
is a nicer way of dealing with files, even the official docs recommend
it.
The commands dictionary
Please go back to my previous answer and re-read this part: it is very important.
-
1\$\begingroup\$ By the way, good programming is hard and requires serious practice, don't feel bad for having made so many mistakes in your programme. \$\endgroup\$Caridorc– Caridorc2015年04月04日 19:45:58 +00:00Commented Apr 4, 2015 at 19:45
-
1\$\begingroup\$ Its ok, I learn from my mistakes and improve :) \$\endgroup\$PyxlWuff– PyxlWuff2015年04月04日 20:09:28 +00:00Commented Apr 4, 2015 at 20:09
-
1\$\begingroup\$ This, I suspect, is because you wrote "2+2" rather than "2 + 2".
String#split
with no args splits on whitespace. \$\endgroup\$ymbirtt– ymbirtt2015年04月04日 20:33:47 +00:00Commented Apr 4, 2015 at 20:33 -
1\$\begingroup\$ It might be worth learning regular expressions for this, and using one to parse out the user's input. Writing an infix calculator is actually surprisingly difficult. \$\endgroup\$ymbirtt– ymbirtt2015年04月04日 20:37:07 +00:00Commented Apr 4, 2015 at 20:37
-
1\$\begingroup\$ @Mrfunny744 you may implement a reverse polish calculator first because it is easier than infix (standard notation). \$\endgroup\$Caridorc– Caridorc2015年04月04日 20:49:50 +00:00Commented Apr 4, 2015 at 20:49
Just looking at this method, you have a lot to improve:
def calc():
while True:
print ("Welcome to PyCALC")
print ("Please type the number for your suggestion.")
print ("1: Addition\n2: Subtraction\n3: Multiplacation\n4: Division")
suggestion = input("Your Choice: ")
if suggestion == "1":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1+num2
print ("Your answer is:")
print (answer)
break
if suggestion == "2":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1-num2
print ("Your answer is:")
print (answer)
break
if suggestion == "3":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1*num2
print ("Your answer is:")
print (answer)
break
if suggestion == "4":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1/num2
print ("Your answer is:")
print (answer)
break
else:
print ("Your operation choice is invalid!")
First, you print the "Welcome to PyCALC" message each time it loops - this should be a one time message displayed when it first is called.
Second, with the exception of the assignment to answer
, you have this section in each of the calculations:
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1+num2
print ("Your answer is:")
print (answer)
break
Move some of this outside the if
:
if suggestion == "1":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1+num2
print ("Your answer is:")
print (answer)
break
while True:
print ("Welcome to PyCALC")
print ("Please type the number for your suggestion.")
print ("1: Addition\n2: Subtraction\n3: Multiplacation\n4: Division")
suggestion = input("Your Choice: ")
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
if suggestion == "1":
answer = num1 + num2
print ("Your answer is:")
print (answer)
break
That has a lot less duplicated code. You should consider splitting it up into a couple methods as well, instead of having everything in one method.
Third, never delay your program like this. Consider if you were marketing this project, and I built an identical one, except I didn't delay - which would you pick as the consumer? Probably the one that was faster.
Fourth, I would probably merge these two print()
s:
print ("Your answer is:")
print (answer)
I think it would look just as nice printed on a single line:
print ("Your answer is:", answer)
Fifth, to really be a useful calculator, you should let me input any basic mathematical equation, like 2 * (2 + 4)
, parse it, and calculate the value. Just as a warning, while you could use eval
to do this, don't - this will execute any legal Python program, which creates the risk of malicious code being run through your calculator. You could add some constants, such as PI
and E
, you could allow calculating powers with the ^
symbol, and much more.
General Notes
Write more functions! You're copy-pasting a lot of stuff around here. If you copy paste anything at all, you should stop for a moment and ask yourself "why can't I make this a function"? Prompting the user for a filename and returning that filename on the end of a path? That's a function. Prompting the user for the contents of a file and replacing its contents with that? That's also a function.
Use modules! https://docs.python.org/2/tutorial/modules.html. Your calculator shouldn't be in the same file as your text editor, you'll just confuse yourself when they start to expand.
Regarding PyCALC:
def calc():
while True:
print ("Welcome to PyCALC")
print ("Please type the number for your suggestion.")
print ("1: Addition\n2: Subtraction\n3: Multiplacation\n4: Division")
suggestion = input("Your Choice: ")
if suggestion == "1":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1+num2
print ("Your answer is:")
print (answer)
break
if suggestion == "2":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1-num2
print ("Your answer is:")
print (answer)
break
if suggestion == "3":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1*num2
print ("Your answer is:")
print (answer)
break
if suggestion == "4":
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
answer = num1/num2
print ("Your answer is:")
print (answer)
break
else:
print ("Your operation choice is invalid!")
Using numbers to encode user input isn't necessarily bad, but your code shouldn't have those repeated throughout it. Consider instead using an Enum, which ships with Python 3, and is pip install
-able in Python>=2.4; https://stackoverflow.com/a/1695250/2581168.
The conditions under which this function is exited aren't clear. Consider using an explicit valid_input
parameter, initialised to False
, which is set to True
when the user inputs something valid.
If the user inputs something valid, all four outcomes are almost completely identical. Consider putting them all into another function, where only that one line is branched.
from enum import IntEnum # run 'pip install enum' first if you're not on Python 3!
class Operation(IntEnum):
add = 0
sub = 1
mul = 2
div = 3
def perform_calculation(op):
num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))
print ("Please wait...")
time.sleep(0.6)
if op == Choice.add:
answer = num1+num2
elif op == Choice.sub:
answer = num1-num2
elif op == Choice.mul:
answer = num1*num2
elif op == Choice.div:
answer = num1/num2
print ("Your answer is:")
print (answer)
break
def calc():
valid_choice = False
while not valid_choice:
print ("Welcome to PyCALC")
print ("Please type the number for your suggestion.")
print ("{0}: Addition\n{1}: Subtraction\n{2}: Multiplacation\n{3}: Division".format(int(Choice.add), int(Choice.sub), int(Choice.mul), int(Choice.div)))
input_op = input("Your Choice: ")
if input_op.isdigit() and int(input_op) in Operation.__members__.values():
perform_calculation(input_op)
valid_choice = True
else:
print ("Your operation choice is invalid!")
It's possibly a bit out of scope, but if you wanted to win brownie points from functional programmers, take a look at the Operator library and see if you can find an even cleaner way of doing this; https://docs.python.org/2/library/operator.html.
First, please use if __name__ == "__main__":
. This will help you if you ever decide to split your program up into multiple files, and I think it is time you did. It is quite cluttered the way it is, and you could have one file per supported feature - one for the calculator, one for the hangman game, etc. You can read specifics about why you should use this here.
Second, please use more methods! I would have one method print the news, one print help prompts, one call the other .EXE files (or pretend .EXE files), and so on. Also, we don't need the welcome each time it loops, it should be displayed one time when the program is opened. If you used more methods, this section would be a lot cleaner, a lot smaller, and a lot easier to debug.
while True:
os.system('cls' if os.name == 'nt' else 'clear')
print ()
print ("PyDOS VERSION 1.9.5")
shell = input("> ")
if shell == "textviewer":
print ("Loading Text Viewer...")
time.sleep(3)
textviewer()
elif shell == "help":
print ("HELP")
print ("-----------------")
print ("Commands:")
print ("dir - displays directory.")
print ("cls - clears screen")
print ("help - shows help")
print ("textviewer - launches textviewer")
print ("edit - launches edit")
print ("news - launches news")
print ("shutdown - closes PyDOS")
print ("calc - launches calculator")
print ("------------------------------------")
print ("What is PyDOS?")
print ("PyDOS is inspired by MS-DOS made in the 1980's and")
print ("has that feel of it too! For a better experiance")
print ("run PyDOS in the python CMD shell.")
input ("Press enter to close.")
elif shell == "edit":
print ("Loading edit...")
time.sleep(3)
edit()
elif shell == "calc":
calc()
elif shell == "dir":
print ("The drive name is A:")
print ()
print ("NAME: TYPE: MODIFIED:")
print ("SHUTDOWN.EXE .EXE 12/01/15 ")
print ("EDIT.EXE .EXE 09/03/15 ")
print ("TEXTVIEWER.EXE .EXE 09/03/15 ")
print ("NEWS.EXE .EXE 09/01/15 ")
print ("HELP.COM .COM 09/03/15 ")
print ("HANGMAN(BROKEN).EXE .EXE 11/03/15 ")
print ("CALC.EXE .EXE 20/03/15 ")
input ("---------------Press enter to close---------------")
elif shell == "cls":
cls()
elif shell == "hangman":
hangman.main()
elif shell == "news":
print ("PyDOS NEWS")
print ()
print ("New Additions:")
print ()
print ("New calculator app!")
print ("All text documents from edit are now stored in a 'files' directory")
print ()
print ("Tweaks and fixes:")
print ()
print ("BUG023L: Fixed issue with splash screen loop")
print ()
print ("Reported Bugs:")
print ("BUG024T: Hangman returns a traceback error, we are fixing this!")
print ("BUG025T: Pressing 'y' in texteditor when it asks you if you want\nto edit the file returns Type_Error")
input("Press enter to close")
elif shell == 'shutdown':
print ("Shutting down...")
time.sleep(3)
break
else:
print("This command or file: "+shell+ " does not exist. Check for spelling errors and try again.\nIf you are trying to open a textfile, open textviewer.")
time.sleep(5)
os.system('cls' if os.name == 'nt' else 'clear')
You could, for example, do this instead:
elif shell == "help":
help()
Third, don't sleep your program. You never want your program lagging for no apparent reason - especially when the user knows it shouldn't.
Fourth, I would print a list of valid inputs when the program is first run, not leave the user looking at a blank screen and wondering what to do next.
HANGMAN(BROKEN).EXE
... you meanHANGMA~1.EXE
:p \$\endgroup\$