7
\$\begingroup\$

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')
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Apr 4, 2015 at 16:06
\$\endgroup\$
6
  • \$\begingroup\$ The calculator immediately clears the screen for me, so I can't see the result! \$\endgroup\$ Commented Apr 4, 2015 at 16:44
  • 1
    \$\begingroup\$ It doesn't appear you took all the advice in this answer. I would recommend you use the dictionary suggested there. \$\endgroup\$ Commented Apr 4, 2015 at 17:01
  • 1
    \$\begingroup\$ I recommend that you read this meta post and add more context to your question. \$\endgroup\$ Commented Apr 4, 2015 at 17:28
  • 3
    \$\begingroup\$ HANGMAN(BROKEN).EXE... you mean HANGMA~1.EXE :p \$\endgroup\$ Commented Apr 4, 2015 at 17:32
  • 1
    \$\begingroup\$ One note: near the top, "Multiplacation" is misspelled. \$\endgroup\$ Commented Apr 5, 2015 at 20:47

4 Answers 4

13
\$\begingroup\$

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 +

CONSTANTly 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.

answered Apr 4, 2015 at 19:45
\$\endgroup\$
12
  • 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\$ Commented Apr 4, 2015 at 19:45
  • 1
    \$\begingroup\$ Its ok, I learn from my mistakes and improve :) \$\endgroup\$ Commented 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\$ Commented 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\$ Commented 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\$ Commented Apr 4, 2015 at 20:49
7
\$\begingroup\$

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.

answered Apr 4, 2015 at 16:38
\$\endgroup\$
0
6
\$\begingroup\$

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.

answered Apr 4, 2015 at 20:08
\$\endgroup\$
5
\$\begingroup\$

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.

answered Apr 4, 2015 at 16:53
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.