2
\$\begingroup\$

Here is the script that I would like reviewed for the following:

  1. Best practices and design pattern usage
  2. Correctness in unanticipated cases
  3. Possible error checking

Any streamline updates/ideas are welcome!

import os
import tarfile
import time

# Create backup filename
timestr = time.strftime("%Y-%m-%d:%H%M%S")
output_filename = "minecraft-backup-" +timestr
backup_filename = output_filename + '.tar.gz'

# Locataion of final backup files
destination_dir = "/Users/username/Desktop/backup/"
destination_file = destination_dir + backup_filename

# Directory to backup
backup_dir = '/Users/username/.config/'

# Backup files
with tarfile.open(destination_file, "w:gz") as tar:
    for fn in os.listdir(backup_dir):
        p = os.path.join(backup_dir, fn)
        tar.add(p, arcname=fn)

# Remove files older than 7 days
current_time = time.time()
daysToDelete = 7

for dirpath,_,filenames in os.walk(destination_dir):
    for f in filenames:
        fileWithPath = os.path.abspath(os.path.join(dirpath, f))
        creation_time = os.path.getctime(fileWithPath)
        print("file available:",fileWithPath)
        if (current_time - creation_time) // (24 * 3600) >= daysToDelete:
            os.unlink(fileWithPath)
            print('{} removed'.format(fileWithPath))
            print("\n")
```
\$\endgroup\$
1
  • \$\begingroup\$ Clearly explain what this does in a sentence or two. It's generally not possible to say if code is working correctly otherwise. \$\endgroup\$ Jun 17 at 16:25

1 Answer 1

1
\$\begingroup\$

For many of your file manipulation operations prefer pathlib over os.

You can pull in your filename formatting including time to one f-string. Prefer datetime over time, and prefer timedelta over a day integer.

Note that the colon in your filename is not Windows-compatible. If you're able, use something like an underscore instead.

tar has built-in support for recursive directory archiving, so you should probably just do that instead of looping.

Suggested

import os
import tarfile
from datetime import datetime, timedelta
from pathlib import Path

now = datetime.now()
backup_filename = f'minecraft-backup-{now:%Y-%m-%d_%H%M%S}.tar.gz'
backup_dir = Path('~/.config').expanduser()
destination_dir = Path('~/Desktop/backup').expanduser()
destination_dir.mkdir(exist_ok=True)
destination_file = destination_dir / backup_filename

with tarfile.open(destination_file, 'w:gz') as tar:
    tar.add(backup_dir)

# Remove files older than 7 days
time_to_delete = timedelta(days=7)

for dirname, _, filenames in os.walk(destination_dir):
    dirpath = Path(dirname)
    for f in filenames:
        file_with_path = dirpath / f
        print('file available:', file_with_path)

        creation_time = datetime.fromtimestamp(
            os.path.getctime(file_with_path)
        )
        if now - creation_time >= time_to_delete:
            # file_with_path.unlink()
            print(file_with_path, 'removed')
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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