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