Misleading global variable

Question

I have an exercise and it is now good and running. The algo of this exercise is to download files from FTP server zip it and upload it again in the upload folder in the FTP server again. BTW this is my code:

import os
import zipfile
import ConfigParser
import ftputil
import shutil
import tempfile
import time


def download_files():
    # Alvin: Think of a better variable name
    file_list = ftp.listdir(downloads)
    for filename in file_list:
        # Alvin should be ftp.path.join
        source = os.path.join(downloads, filename)
        target = os.path.join(temp_path, filename)
        ftp.download(source, target)

def zipping_files():
    dirlist = os.listdir(temp_path)
    global filepath
    filepath = os.path.join(temp_path, 'part2b.zip')
    # Alvin: find better name for zip_name
    global zip_name
    zip_name = zipfile.ZipFile(filepath, 'w')
    # Alvin: try not to use built-in names as variable names
    for list in dirlist:
        # Alvin: file_path, absolute_path
        get_file = os.path.join(temp_path, list)
        zip_name.write(get_file, 'part2b\\' + list)

def upload_files():
    ftp.upload(filepath, uploads + '/part2b.zip')

def main():
    # Alvin: Do not use globals.
    # Alvin: remove useless and above all misleading comments

    global temp_path
    temp_path = tempfile.mkdtemp()


    config = ConfigParser.ConfigParser()
    config.readfp(open('config.ini'))
    server = config.get('main', 'Server')
    username = config.get('main', 'Username')
    password = config.get('main', 'Password')

    global uploads
    uploads = config.get('main', 'Upload folder')

    global downloads
    downloads = config.get('main', 'Download folder')

    #connect to ftp
    global ftp
    # Alvin: open ftp connection when you only need it.
    ftp = ftputil.FTPHost(server, username, password)

    try:
        download_files()
        zipping_files()
        upload_files()

    finally:
        ftp.close()
        zip_name.close()
        shutil.rmtree(temp_path, 'true')

    print "Successfully transferred files."
    print ""
    print "This will close in 5 seconds. . . . ."
    time.sleep(5)

if __name__ == '__main__':
    main()

Alright, the naming conventions leave that to me. But I want to ask for an example of how can I recode it without using all of my global variables? Thanks for your help!

Answer 1

You should definitly give parameters to your methods.

Here is how you can proceed:

1. Identify what your method are doing, each one should have only one precise goal
2. Identify what is needed for that purpose, and put it in arguments list
3. Identify what it shall return and return it
4. Your main function is fine, and is the perfect place to centralize those variables

One of the very best point of not using globals and having one function/one method is that it will be very easy to test/debug when something goes wrong.

Example:

your download_files() requires downloads and temp_path, make them arguments, not globals

Answer 2

You have four globals in your main as of now. (temp_path, uploads, downloads and ftp).

First, remove (comment) the lines with global, and then these 4 variables would turn local to main. But that would make them not accessible to the other functions. So, you need to pass them to the functions.

For that purpose, you can add these variables as parameters to the functions which require them. So, for example, you try block would change to..

try:
    download_files(downloads, temp_path)
    zipping_files(temp_path)
    upload_files(ftp)

Now, to match the addition of the parameters, you shall change the functions too. The signatures of the functions which are called from the try block would be:

def download_files(downloads, temp_path):

def zipping_files(temp_path):

def upload_files(ftp):

Similarly, you can remove the globals you have in other functions too (eg. global filepath in zipping_files()).

Answer 3

Thanks guys for all your help!! All your answers are very helpful! This is my final and running code:

import os
import zipfile
import ConfigParser
import ftputil
import shutil
import tempfile
import time


def download_files(downloads, temp_path, server, username, password):
    ftp = ftputil.FTPHost(server, username, password)
    file_list = ftp.listdir(downloads)
    for filename in file_list:
        source = os.path.join(downloads, filename)
        target = os.path.join(temp_path, filename)
        ftp.download(source, target)
    ftp.close()

def zipping_files(temp_path):
    file_list = os.listdir(temp_path)
    filepath = os.path.join(temp_path, 'part2b.zip')
    zip_file = zipfile.ZipFile(filepath, 'w')
    for filename in file_list:
        file_path = os.path.join(temp_path, filename)
        zip_file.write(file_path, 'part2b\\' + filename)
    zip_file.close()

def upload_files(uploads, temp_path, server, username, password):
    ftp = ftputil.FTPHost(server, username, password)
    filepath = os.path.join(temp_path, 'part2b.zip')
    ftp.upload(filepath, uploads + '/part2b.zip')
    ftp.close()

def main():

    temp_path = tempfile.mkdtemp()

    config = ConfigParser.ConfigParser()
    config.readfp(open('config.ini'))
    server = config.get('main', 'Server')
    username = config.get('main', 'Username')
    password = config.get('main', 'Password')
    uploads = config.get('main', 'Upload folder')
    downloads = config.get('main', 'Download folder')

    try:
        download_files(downloads, temp_path, server, username, password)
        zipping_files(temp_path)
        upload_files(uploads, temp_path, server, username, password)

    finally:    
        shutil.rmtree(temp_path, 'true')

    print "Successfully transferred files."
    print ""
    print "This will close in 5 seconds. . . . ."
    time.sleep(5)

if __name__ == '__main__':
    main()

Any suggestions are all will be consider! Thanks again!

Answer 4

Python doesn't really support global variables – they are generally a bad idea. Either pass the data around as arguments, or wrap your functions in a class, and use the parameters as class members.

(Technically, python has the global keyword, but you have to use it in every function for every global variable. It's as ugly as it should be. Don't use it.)