HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Multiple server FTP download class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ftpdownloadmultipleserverclass

Problem

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)

Solution

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.

Context

StackExchange Code Review Q#108681, answer score: 2

Revisions (0)

No revisions yet.