background:
I have a model called FileInfo
, which represents a sftp file / windows dfs file.
class FileInfo(object):
def __init__(self, server_info, folder_path, file_name):
self.server_info = server_info
self.folder_path = folder_path
self.file_name = file_name
def get_file_full_path(self):
return os.path.join(self.folder_path, self.file_name)
FileInfo
has a ServerInfo
to hold the server connection info: e.g. sftp connection details.
class ServerInfo(object):
pass
class WindowsServerInfo(ServerInfo):
pass
class SFTPServerInfo(ServerInfo):
pass
this is how I print out the full path:
if __name__ == '__main__':
sftp_server_info = SFTPServerInfo()
win_server_info = WindowsServerInfo()
print FileInfo(win_server_info, r'\\dfs\domain\tmp', 'test.txt').get_file_full_path()
print FileInfo(sftp_server_info, r'/tmp', 'test.txt').get_file_full_path()
the application is running in Windows and Linux, so the output of the above code will be different -- depends on the hosting OS result on Linux
:
\\dfs\domain\tmp/test.txt
/tmp/test.txt
result on Windows
:
\\\\dfs\\domain\\tmp\\test.txt
/tmp\\test.txt
I want to have a consistent output, such as:
\\dfs\domain\tmp\test.txt
/tmp/test.txt
so I start to refactor the code:
v1. if...else...
class FileInfo(object):
def get_file_full_path(self):
if isinstance(self.server_info, SFTPServerInfo):
return r'%s/%s' % (self.folder_path, self.file_name)
elif isinstance(self.server_info, WindowsServerInfo):
return r'%s\%s' % (self.folder_path, self.file_name)
else
raise exception
it works, but if...else means it'll be a big method if I want to support more server types. so I'm going to create a dict to handle it.
v2. a dict contains ServerInfo type and function mapping
class FileInfo(object):
def get_file_full_path(self):
return get_full_path_functions[type(self.server_info)](self.folder_path, self.file_name)
def get_full_path_windows(folder_path, file_name):
return r'%s\%s' % (folder_path, file_name)
def get_full_path_sftp(folder_path, file_name):
return r'%s/%s' % (folder_path, file_name)
get_full_path_functions = {
WindowsServerInfo: get_full_path_windows,
SFTPServerInfo: get_full_path_sftp
}
it works fine, but I think it may be a good idea to move these methods into ServerInfo
v3. instance method
class FileInfo(object):
def get_file_full_path(self):
return self.server_info.get_full_path(self.folder_path, self.file_name)
class WindowsServerInfo(ServerInfo):
def get_full_path(self, folder_path, file_name):
return r'%s\%s' % (folder_path, file_name)
class SFTPServerInfo(ServerInfo):
def get_full_path(self, folder_path, file_name):
return r'%s/%s' % (folder_path, file_name)
however, get_full_path
methods are not using self
, and this method should not belongs to any instance of ServerInfo
v4. static method
class FileInfo(object):
def get_file_full_path(self):
return type(self.server_info).get_full_path(self.folder_path, self.file_name)
class WindowsServerInfo(ServerInfo):
@staticmethod
def get_full_path(folder_path, file_name):
return r'%s\%s' % (folder_path, file_name)
class SFTPServerInfo(ServerInfo):
@staticmethod
def get_full_path(folder_path, file_name):
return r'%s/%s' % (folder_path, file_name)
All these 4 versions are working as expected, please help to review them and let me know:
- from the OOP perspective, which one is the best? is there a better way to desing it?
- from Python perspective, which one is the most pythonic way?
-
\$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$Jamal– Jamal2016年05月24日 04:06:44 +00:00Commented May 24, 2016 at 4:06
3 Answers 3
Since you have classes that represent the configuration of various server types, you should keep the path structure in them and uses it from them. pathlib
provide an abstraction over windows and unix file paths that you can store in these classes and reuse it from FileInfo
.
import pathlib
class FileInfo(object):
def __init__(self, server_info, folder_path, file_name):
self.server_info = server_info
self.folder_path = folder_path
self.file_name = file_name
def get_file_full_path(self):
return self.server_info.paths(self.folder_path, self.file_name)
class ServerInfo(object):
pass
class WindowsServerInfo(ServerInfo):
paths = pathlib.PureWindowsPath
class SFTPServerInfo(ServerInfo):
paths = pathlib.PurePosixPath
However, if the only purpose of the FileInfo
class is to retrieve a path from its arguments, then you’re better of putting this function directly into ServerInfo
:
class ServerInfo(object):
def get_file_full_path(self, folder_path, file_name):
return self.paths(folder_path, file_name)
class WindowsServerInfo(ServerInfo):
paths = pathlib.PureWindowsPath
class SFTPServerInfo(ServerInfo):
paths = pathlib.PurePosixPath
And you could improve it by allowing multiple path segments:
class ServerInfo(object):
def get_file_full_path(self, *path_segments):
return self.paths(*path_segments)
class WindowsServerInfo(ServerInfo):
paths = pathlib.PureWindowsPath
class SFTPServerInfo(ServerInfo):
paths = pathlib.PurePosixPath
But it seems that you're only using Python 2, so pathlib
may not be available. You can still use your '%s/%s'
or '%s\\%s'
style approach, but you will loose some path simplifications provided by pathlib
:
class ServerInfo(object):
def get_file_full_path(self, *path_segments):
return self.path_delimiter.join(*path_segments)
class WindowsServerInfo(ServerInfo):
path_delimiter = '\\'
class SFTPServerInfo(ServerInfo):
path_delimiter = '/'
Oh, and talking about '%s/%s'
or '%s\\%s'
, str.format
is the prefered way over %
formating syntax.
from the OOP perspective, which one is the best? is there a better way to desing it?
If I have to choose from v1 to v4, I prefer v3. As you ask from the OO perspective, only v3 exhibits polymorphism, one of the core characteristics of OO.
however, get_full_path methods are not using self, and this method should not belongs to any instance of ServerInfo
Yes, that's a valid concern. It usually signals that you should create instances instead of classes. So if I were you, I'll write something like:
class ServerInfo(object):
path_format
def get_full_path(folder_path, file_name):
return path_format % (folder_path, file_name)
def __init__(self, path_format):
self.path_format = path_format
@staticmethod
def window_server_info_singleton():
return new ServerInfo('%s\%s') # TODO: cache it
def unix_server_info...
from Python perspective, which one is the most pythonic way?
If the paths calculated are to be used by the client machine, then probably most of the classes can be eliminated.
class FileInfo(object):
def get_file_full_path(self):
return '%s\%s' if sys.platform.startswith('win') else '%s/%s'
-
\$\begingroup\$ Didn't downvote, but why do you prefer v3? You don't tell us why it is better than the other options. \$\endgroup\$syb0rg– syb0rg2016年05月23日 16:10:59 +00:00Commented May 23, 2016 at 16:10
-
\$\begingroup\$ As he asks from the OO perspective, only v3 exhibits polymorphism, one of the core characteristics of OO. \$\endgroup\$Jack– Jack2016年05月24日 01:41:40 +00:00Commented May 24, 2016 at 1:41
-
\$\begingroup\$ That should be clarified in your answer, not appended as a comment \$\endgroup\$syb0rg– syb0rg2016年05月24日 03:05:16 +00:00Commented May 24, 2016 at 3:05
IMHO, OS dependence is in behavior, not in the class state. So modeling the differences in the class hierarchy may not be desirable. Maybe your code can detect the OS environment early and dispatch to different functions that represent the behaviors.
def get_full_path(os_env):
return {
'Windows': lambda : ...,
'Sftp': lambda : ...
}.get(os_env)()