6
\$\begingroup\$

I made this program to automate/simplify the erasing of a lot of files, I have some ideas to improve it but that's not why I'm putting it here, it's because it's a very dangerous program(it deletes stuff permanently) and since I'm a beginner(ish) I want some advice from better programmers to improve it.

import os
import send2trash
import shutil
file = input("Type the file path")
while os.path.exists(file) is False:  # validation
    file = input("Path not found, try again:")
for i, j, k in os.walk(file):  # files check
    print("The folder is in this path:", i)
    print("The subfolders are named:", j)
    print("The files are named:", k)
while True:
    try:
        command = int(input("1 - Delete permanently the file\n2 - Send the file to the bin"))
    except ValueError:
        print("Just the numbers 1 or 2 are accepted, follow the instructions")
        continue
    if command not in range(1, 3):
        print("Just the numbers 1 or 2 are accepted, follow the instructions")
        continue
    else:
        break
if command == 1:
    try:
        os.rmdir(file)  # deletes if there is no inside folder
    except OSError:
        shutil.rmtree(file)  # deletes if there is inside folders
else:
    send2trash.send2trash(file)  # send to the bin
\$\endgroup\$
3
  • 3
    \$\begingroup\$ What if I type a path and then I see that it still contains some files I want to retain? (Good that you had printed them). But then I'd like to abort, maybe by typing 0 - currently my only option is to kill the program. \$\endgroup\$
    – Bergi
    Dec 21, 2017 at 20:05
  • \$\begingroup\$ @Bergi Ctrl-C will raise a KeyboardInterrupt. \$\endgroup\$
    – Daniel
    Dec 21, 2017 at 22:58
  • \$\begingroup\$ @Bergi I already added the close the program option thanks! \$\endgroup\$
    – Mateus
    Dec 23, 2017 at 5:36

2 Answers 2

11
\$\begingroup\$

There are some things that can be improved here

  • Divide your code into functions, don't put everything in the global namespace!

  • Use a if __name__ == '__main__' guard.

  • It's not bad to give your code a little more space in the sake of readability, a few blank lines would help make this code look better.

    Read PEP8#blank-lines for more details.

  • Don't use == or is for False, True comparison

    See PEP8#programming-recommendations.

    while os.path.exists(file) is False:
    

    You can rewrite to a more idomatic while not os.path.exists(file):

  • Naming is important

    file = input("Type the file path")
    

    This is a path, so rename it to path = ...

    for i, j, k in os.walk(file):
    

    You could name those to what they represent:

    for folder, subfolder, files in os.walk(path):
    
\$\endgroup\$
2
  • \$\begingroup\$ thanks for the readability tips, I suppose there is no danger or inefficiency in the code yet right? \$\endgroup\$
    – Mateus
    Dec 21, 2017 at 9:41
  • 1
    \$\begingroup\$ @Mateus I have not spotted something major yet, apart from putting everything in the global namespace, this looks pretty OK. \$\endgroup\$
    – Ludisposed
    Dec 21, 2017 at 10:04
4
\$\begingroup\$

Unless this is just a homework or learning problem, I would do the following in addition to the accepted answer.

  1. I would use command line args instead of prompts. This will make it potentially usefull as an automation tool for you later , look at the begins library, it makes this trivial.

  2. I would absolutely add checks for critical file paths. It is highly unlikely this tool will be used to delete kernel files or program files (windows),, so it shouldn't be allowed to do so unless explicitly requested.

  3. Like #2, deleting hidden files should be an explicit request.

  4. You should create a history file of what has been deleted, so as last resort the user knows what just happened.

  5. The program shouldn't be able to delete itself.

  6. It should be possible to turn off delete confirmation, in case the tool is to be used in automation.

  7. If you really want to be safe, implementing an undo will save your bacon at least once.

Given the potential of these types of applications to brick a user's OS, it should really be treated with extreme care.

\$\endgroup\$

Your Answer

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

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