4
\$\begingroup\$

This script, once added to the Windows Scheduler, automatically creates a backup of your Google Chrome bookmarks.

import shutil
import os
import datetime


def setup():
    print ('Enter your Chrome bookmark directory.')
    print ('Standard one is: C:\\Users\\<USER>\\AppData\\Local\\Google\\Chrome\\User Data\\Default')
    bookmarkfolder = input('> ')

    print ('Enter the folder you want to save the backup files in.')
    print ('For example: C:\\Users\\<USER>\\Desktop\\Chrome Bookmarks')
    backupfolder = input('> ')

    with open('user.txt', 'w') as txt:
        txt.write(bookmarkfolder + '\n' + backupfolder)

    copy()


def copy():
    userList = []
    date = datetime.date.today()

    with open('user.txt', 'r') as txt:
        for line in txt:
            userList.append(line.strip())

    shutil.copy(userList[0] + '\\Bookmarks', userList[1])
    try:
        os.rename('Bookmarks', 'Bookmarks' + str(date).replace('-',''))
    except WindowsError:
        os.remove('Bookmarks' + str(date).replace('-',''))
        os.rename('Bookmarks', 'Bookmarks' + str(date).replace('-',''))


def checksetup():
    with open('user.txt', 'r') as setupfile:
        setupfile.seek(0)
        for line in setupfile:
            if '\\' in line:
                copy()
            else:
                setup()

checksetup()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Hi! Welcome to Code Review. Please add more context to your question, especially explain what your code does. \$\endgroup\$ Commented Oct 26, 2015 at 1:17

2 Answers 2

2
\$\begingroup\$

The logic isn't so easy to follow, especially since the loops in copy and checksetup imply that there are more lines, but really there will be only two at most, the bookmark and backup folder. I'd suggest making that more obvious and avoiding for line in file: in case you only ever want to have a fixed number of lines in that file.

The code also doesn't work with no "user.txt" file present, so some more checks and error handling are necessary to handle that case as well.

Some of the paths and other constructs are duplicated, I'd always try to minimise the amount of duplication, so with that in mind, consider more constants and variables to extract common code.

  • The .seek(0) in checksetup isn't necessary with mode 'r'; if you open a file (for reading) you'll already be at the start; however, the mode should probably be 'a+' anyway to create the file if it doesn't exist.
  • If you don't like the escapes all the time, consider using the r"" syntax instead.
  • Use the if __name__ == "__main__": ... construct as is customary, that way this file could be imported and used without running checksetup as is it would currently.
  • Python typically uses lower case names with underscores between words, c.f. PEP8.
  • For the date formatting use the strftime method instead to get the desired output directly.
  • The setup method could use some more error checks, e.g. whether the folders exist.
  • The "user.txt" file is opened three times in the worst case; it would also be better to pass the two values necessary for the script in the script as function arguments instead of parsing the file so often; I'll leave that be for the moment though. In general I'd say that copy should have the signature copy(bookmark_directory, backup_directory) and should be called with the values from the file instead of parsing it.

After all that you can also look into command line arguments using e.g. argparse for parsing options, that way users won't have to deal with pesky manual input instead of reusing the shell completion (for specifying the folders).

So, it should look more like this perhaps:

import shutil
import os
import datetime


SETUP_FILE = 'user.txt'


def setup():
    print ('Enter your Chrome bookmark directory.')
    print (r'Standard one is: C:\Users\<USER>\AppData\Local\Google\Chrome\User Data\Default')
    bookmarkfolder = input('> ')

    print ('Enter the folder you want to save the backup files in.')
    print (r'For example: C:\Users\<USER>\Desktop\Chrome Bookmarks')
    backupfolder = input('> ')

    with open(SETUP_FILE, 'w') as txt:
        txt.write(bookmarkfolder + '\n' + backupfolder + '\n')

    copy()


def copy():
    with open(SETUP_FILE, 'r') as txt:
        user_list = [line.strip() for line in txt]

    shutil.copy(user_list[0] + r'\Bookmarks', user_list[1])

    new_name = datetime.date.today().strftime('Bookmarks%Y%m%d')
    try:
        os.rename('Bookmarks', new_name)
    except WindowsError:
        os.remove(new_name)
        os.rename('Bookmarks', new_name)


def checksetup():
    try:
        with open(SETUP_FILE, 'r') as txt:
            txt.seek(0)
            line = txt.readline()

        if '\\' in line:
            copy()
        else:
            setup()
    except FileNotFoundError:
        setup()


if __name__ == "__main__":
    checksetup()
\$\endgroup\$
2
\$\begingroup\$

You could make userList with a list comprehension. That's basically when you create a list from a for loop like expression. Yours would look pretty simple:

with open('user.txt', 'r') as txt:
    userList = (line.strip() for line in txt)

That said, why is it called userList? That makes it sound like it's a list of users, but it's not at all. It's a two item list of user defined values. You should name something to indicate what it contains or what it's used for. Neither of these purposes match your userList name. You could instead indicate how they're folders, with a name like folders.

Currently you have it hardcoded to only write two lines, if that's always the intended case then you don't even need to make a list. You could use Python's tuple assignment and do this:

    source_dir, backup_dir = (line.strip() for line in txt)

Where source_dir is the first line and backup_dir is the second line. They're entirely separate variables that you can now use instead of having to index a list:

shutil.copy(source_dir + '\\Bookmarks', backup_dir)

Also you shouldn't concatenate strings to make paths. The os module has a method for this. os.path.join(source_dir, "Bookmarks") will do the job for you. And since it's the os module, it will choose the right path separator for the OS the user is on.

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