2
\$\begingroup\$

I am new to programming and need input on how I could've written something better. Not coding on daily basis. The task was to write a script in python, bash or anything. So I mixed python with bash commands using python module os.systems. Some friends recommended lists but I wanted the output to be similar as the example in this task:

Write a script (Bash, Python etc.) that checks for files - in directory X - that have not been modified in (older than) the last Y days.
The directory and days should be passed to the script as mandatory arguments.

The script shall only look for files in directory X, not in sub directories. You may assume that none of the filenames contain newlines.

The output of the script should print the file names, and the time for when the file was last modified, sorted by modification time:

./README.txt                      2019-05-09  17:19:53.193771720  +0200
./README.txt.gpg                  2019-05-09  17:20:21.331833720  +0200
./migratemost-master.zip          2019-05-20  12:52:34.867119547  +0200
./INC177759                       2019-05-23  13:29:47.014557386  +0200

Include a help option, so that if '-h' or '--help' is passed as an optional argument, a summary of what the program does is printed to stdout.

Also try to handle user errors so the script exits gracefully with an error message upon incorrect - or missing - input.

My solution that I need help to refactor/improve peer review on:

import os
import sys

days = raw_input("Please enter days: ")
if days.isdigit():
    print "You entered:", days
else:
    sys.exit("Exiting the program, wrong data type.")

dir = raw_input("Please enter path: ")
print "You entered:", dir

stuff_in_string = "find {} -type f -mtime -{}".format(dir, days)
print stuff_in_string

print(os.system('{} | xargs -d \'\n\' ls -lth --full-time'.format(stuff_in_string)))

The output:

$ python main.py
Please enter days: asdf
Exiting the program, wrong data type.
$
$ python main.py
Please enter days: 365
You entered: 365
Please enter path: .
You entered: .
find . -type f -mtime -365
-rw-r--r--. 1 root root 413 2020-09-21 16:43:48.608029286 +0200 ./main.py
-rw-r--r--. 1 root root   0 2020-09-21 09:36:17.072137720 +0200 ./file2.csv
-rw-r--r--. 1 root root   0 2020-09-21 09:35:28.502502950 +0200 ./file1.txt
0
$
$ python main.py
Please enter days: 9999
You entered: 9999
Please enter path: .
You entered: .
find . -type f -mtime -9999
-rw-r--r--. 1 root root 413 2020-09-21 16:43:48.608029286 +0200 ./main.py
-rw-r--r--. 1 root root   0 2020-09-21 09:36:17.072137720 +0200 ./file2.csv
-rw-r--r--. 1 root root   0 2020-09-21 09:35:28.502502950 +0200 ./file1.txt
-rw-r--r--. 1 root root   0 2012-01-01 00:00:00.000000000 +0100 ./goldenfile.xls
0
\$\endgroup\$
5
  • \$\begingroup\$ Not been modified in last 1 year, should not include main.py from my perspective \$\endgroup\$
    – hjpotter92
    Sep 25, 2020 at 7:37
  • \$\begingroup\$ main.py is arbitrary. Not been modified in last 1 year? \$\endgroup\$ Sep 25, 2020 at 7:42
  • \$\begingroup\$ Welcome to CodeReview@SE. I wanted the output to [look similar to] the example have another look: the example lists file modified more than 365 days ago, in order of decreasing "age of modification". Your output shows "younger" files increasing in age. \$\endgroup\$
    – greybeard
    Sep 25, 2020 at 7:50
  • \$\begingroup\$ 2020-09-21 16:43:48.608029286 +0200 is the time file was last modified. I do not think this is outside of 1 year limit. \$\endgroup\$
    – hjpotter92
    Sep 25, 2020 at 7:51
  • \$\begingroup\$ (I removed the bash tag, as I hold neither find nor ls to be bash built-ins.) \$\endgroup\$
    – greybeard
    Sep 25, 2020 at 7:53

1 Answer 1

2
\$\begingroup\$

Don't mix programming language

Try to avoid mixing different languages in one program. You are creating a Frankenstein monster this way, that depends on you having to deal with multiple languages, their interaction with each other, and now your computer needs to have the necessary interpreters and libraries installed for both languages. It is also quite expensive to call os.system(), so performance suffers as well.

For the rest of the review I'm assuming you want to want to continue using Python.

Make sure your script works with Python 3

Python version 2 is obsolete, you should move to Python 3. The most important change in your script though is adding parentheses to the print-statements.

Ensure you implement your program according to the requirements

The requirements you mention say that the directory and number of days should be passed as arguments. That means you should not ask for them while the program is running. The easiest way to parse command line arguments is to use the argparse module. As a bonus, this will also automatically create a help text that will be printed when you specify the --help option on the command line.

The requirements also say you have to sort the output based on modification time. Note that your original shell command did not guarantee that the output was correctly sorted: xargs might call ls multiple times, each time with only part of the input.

Have Python produce a list of files

You can use os.listdir() to get a list of all the elements in a directory. That list might also contain the names of subdirectories, but it will not recurse into them. Use os.stat() on each element to check if it is a file or a directory, and also to get the modification time.

Add a shebang line to the top of your script

Add a shebang line to the top, so you can call your script without having to call the Python interpreter explicitly. This line can look like:

#!/usr/bin/python3

I also recommend you remove the .py extension from your script, as it will be a stand-alone script, and for the user it shouldn't matter if it's writting in Python or any other language. This way, along with the argument parsing, you should be able to call your script like so:

$ ./main . 365
./README.txt                      2019-05-09  17:19:53.193771720  +0200
./README.txt.gpg                  2019-05-09  17:20:21.331833720  +0200
./migratemost-master.zip          2019-05-20  12:52:34.867119547  +0200
./INC177759                       2019-05-23  13:29:47.014557386  +0200

Of course, the name main doesn't really tell what your script does. Give it a better name, like list_recent_files.

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