patternpythonMinor
Multiple server FTP download class
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)
```
#!/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.
Is there a way to check if this is file and download instead of trying to treat it as directory -
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
-
Use some constant like
-
Use
-
Don't swallow exceptions - log them, use extended
-
Minor - use
-
Small comment - you are hard-coding logic on where to download files on local disk inside
- 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.