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')
\$\endgroup\$
6
  • \$\begingroup\$ The calculator immediately clears the screen for me, so I can't see the result! \$\endgroup\$
    – Veedrac
    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\$
    – user34073
    Apr 4, 2015 at 17:01
  • 1
    \$\begingroup\$ I recommend that you read this meta post and add more context to your question. \$\endgroup\$
    – SirPython
    Apr 4, 2015 at 17:28
  • 3
    \$\begingroup\$ HANGMAN(BROKEN).EXE... you mean HANGMA~1.EXE :p \$\endgroup\$ Apr 4, 2015 at 17:32
  • 1
    \$\begingroup\$ One note: near the top, “Multiplacation” is misspelled. \$\endgroup\$
    – alexwlchan
    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.

\$\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\$
    – Caridorc
    Apr 4, 2015 at 19:45
  • 1
    \$\begingroup\$ Its ok, I learn from my mistakes and improve :) \$\endgroup\$
    – PyxlWuff
    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
    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
    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
    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.

\$\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.

\$\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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.