Multiple server FTP download class

Question

I've written this a short time ago. Do you see anywhere I can improve my logic? This is my first python script.

#!/usr/bin/python
# -*- coding: utf-8 -*-
import os
from ftplib import FTP # ftplib comes with the py install
class Ftp:
 def __init__(self, hostname, username, password, port=21):
 print '\nConnecting to %s on port %d' % (hostname, port)
 ftp = FTP()
 try:
 ftp.connect(hostname, port)
 except:
 raise Exception('\n\nConnection failed: %s:%d' % (hostname, port))
 try:
 ftp.login(user=username, passwd=password)
 self.start = ftp.pwd()
 print 'Login Successful: %s\n' % username
 except: # ftplib.error_perm
 raise Exception('\n\nAuth: \n%s@%d\n\n' % (hostname, username))
 self.ftp = ftp
 self.hostname = hostname
 def end(self):
 try:
 self.ftp.quit()
 except:
 pass
 print '\n\nConnection to %s has been closed\n' % self.hostname
 def ls(self):
 return self.ftp.nlst()
 def get(self, file):
 return self.ftp.retrbinary(
 'RETR ' + file, open('%s/%s' % (self.hostname, file), 'wb').write)
 def cwd(self, file):
 return self.ftp.cwd(file)
 def pwd(self):
 return self.ftp.pwd()
 def download(self):
 print 'Downloading Files:'
 self._dl()
 print '\nAll Files Downloaded'
 self.end()
 def _dl(self, changedir='/'):
 if changedir is not '/':
 self.cwd(changedir)
 current = self.pwd()
 folders = []
 files = self.ls()
 for file in files:
 try:
 self.cwd(file)
 if current is not '/':
 path = '%s%s/%s' % (host, current, file)
 else:
 path = '%s%s%s' % (host, current, file)
 if not os.path.exists(path):
 os.makedirs(path)
 if path != '%s/.' % host:
 folders.append(path)
 self.cwd(current)
 except:
 if current is not '/':
 self.get('%s/%s' % (current, file))
 else:
 self.get('%s' % file)
 print ' %s' % file
 try:
 for fold in folders:
 print ' /%s' % fold
 self._dl(fold.replace(host, ''))
 except:
 pass

Usage:

hosts = ['domain01.com', 'domain02.com']
user = 'username'
password = 'secr3t'
for host in hosts:
 Ftp(host, user, password).download()

I tried to make it as PEP8 compliant as it can be. It is a short file and straightforward. I need better commenting.

Answer 1

Overall - looks good and readable. Just a few comments.

• I didn't quite get it but looks like you are relying on exception to be thrown to get a signal and download file - this is bad and expensive. Is there a way to check if this is file and download instead of trying to treat it as directory - cwd - and catch exception. Is this what you are trying to do?

In general it is bad to organize program workflow around exceptions - they are for exceptional situations - not for routine work.

• Use os.path.join to concatenate paths - link

• Use some constant like ROOT_PATH instead of hard-coding '/'

• Use logging module for debug/error logging - this is industry standard

• Don't swallow exceptions - log them, use extended except:

  except Exception as e:

• Minor - use "{0}{1}/{2}".format(host, current, file) instead of % notation as this is sometimes wonky and format is just better and industry standard.

• Small comment - you are hard-coding logic on where to download files on local disk inside Ftp class which is working but not the best - just add optional parameter to be able to override root directory where to download data to.

Answer 2

You did a good job following the PEP, stylewise you're matching it for the most part. Though as Maxim pointed out you should use modern formatting. I have some small notes about things.

I'm confused about this inline comment here:

 except: # ftplib.error_perm

Is that the error you're excepting? Then you should explicitly use it:

 except ftplib.error_perm:

You could also raise this specifically rather than a plain Exception:

 raise ftplib.error_perm('\n\nAuth: \n%s@%d\n\n' % (hostname, username))

This will raise it as the correct error and still print the string you're passing as the explanatory message.

If the comment is supposed to mean something else... then what? It's not clear from context, so it's not doing its job.

The bare except in end is definitely a bad idea. It will ignore any error that possibly arises when trying to quit the FTP, and then just tell the user that it closed fine. This is a bad idea generally but especially if you're not closing connections correctly. For instance, if you had made a typo and just written self.fto.quit() then your except would be ignoring it, and you won't be indicating that it happened at all.

Don't use is or is not for string comparison. It will often work due to something known as string interning, but there are cases where it can fail. The keyword is allows you to check identity. This means you're trying to identify if two things are the same, not just that they're equal. This is best used with is None, or when trying to tell if two names refer to the same object. Strings aren't a good use case because though it will often produce correct results it is unreliable and context dependent. Instead just use == and !=.

Avoid using file as a name. It's also the name of the builtin file constructor, so you're shadowing it by using it in your for loop.

Also you could shorten this with a ternary:

if current is not '/':
 path = '%s%s/%s' % (host, current, file)
else:
 path = '%s%s%s' % (host, current, file)

A ternary expression is basically an expression that will return one of two values, based on a tested expression. In your case you can test if current == '/'. It seems backwards to use the negative case here, so I flipped it.

path = ('{}{}{}' if current == '/' else '{}{}/{}').format(host, current, file)

This will use either the string '{}{}{}' or '{}{}/{}' depending on whether or not current == '/'. It's debatable whether or not this is clearer or more readable, but I think it's better for indicating what difference that result actually makes.