2

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!

VisioN
  • 143,310
  • 32
  • 282
  • 281
neo
  • 293
  • 3
  • 7
  • 21
  • Pass these variables around as arguments. – georg Jul 04 '12 at 07:41
  • I'll try that suggestion, actually I have that in my mind but I am not sure if I will declare the variable first? or is it okay that it is declared in my main() function, sorry Im new in python. Thanks anyway! – neo Jul 04 '12 at 07:43
  • In python, you don't have to declare variables prior to assigning them values. – Joel Cornett Jul 04 '12 at 08:01

4 Answers4

4

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

Antoine Pelisse
  • 12,871
  • 4
  • 34
  • 34
  • That works if the `try:`/`finally:` is split up and closing of stuff like zip happens where it is opened. – glglgl Jul 04 '12 at 07:44
  • Well, yeah, obviously `zipping_files()` is opening the `zip_name` and it should be responsible to close it – Antoine Pelisse Jul 04 '12 at 07:47
  • Okay, I will try to use arguments in my functions. I will update you guys whenever Im done. Thanks for all your help! – neo Jul 04 '12 at 07:48
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()).

Dr. S
  • 123
  • 6
0

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!

neo
  • 293
  • 3
  • 7
  • 21
-1

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.)

Creshal
  • 493
  • 1
  • 4
  • 15
  • This answer is both incorrect and a bad advice. You *don't* have to use the keyword to use a global variable, only to change it. And yes, it actually is useful to have global (i.e. module) variables. – PierreBdR Jul 04 '12 at 08:07