2
\$\begingroup\$

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:

  1. from the OOP perspective, which one is the best? is there a better way to desing it?
  2. from Python perspective, which one is the most pythonic way?
\$\endgroup\$
  • \$\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 May 24 '16 at 4:06
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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'
\$\endgroup\$
  • \$\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 May 23 '16 at 16:10
  • \$\begingroup\$ As he asks from the OO perspective, only v3 exhibits polymorphism, one of the core characteristics of OO. \$\endgroup\$ – Jack May 24 '16 at 1:41
  • \$\begingroup\$ That should be clarified in your answer, not appended as a comment \$\endgroup\$ – syb0rg May 24 '16 at 3:05
-2
\$\begingroup\$

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)()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.