Background:
I have made two classes. The first is SharePointHandler
, and contains a few functions that I intend on using to automate some actions on SharePoint. A simplified version of this is shown below:
from office365.runtime.auth.authentication_context import AuthenticationContext
from office365.sharepoint.client_context import ClientContext
from office365.sharepoint.folders.folder import Folder
from office365.sharepoint.files.file import File
from path_handler import PathHandler
class SharePointHandler(object):
def __init__(self, username:str, password:str, company_site:str) -> None:
self.username = username
self.password = password
self.company_site = company_site
self.client_context:ClientContext
self.create_client_context()
def create_client_context(self) -> None:
try:
ctx_authorization = AuthenticationContext(self.company_site)
ctx_authorization.acquire_token_for_user(self.username, self.password)
self.client_context = ClientContext(self.company_site, ctx_authorization)
print(f"\nSharePoint authentication successful.")
except Exception as e:
print(f"\nSharePoint authentication failed. The following exception has occured:\n{e}\n")
def map_folder(self, to_map:PathHandler) -> tuple[list[PathHandler], list[PathHandler]]:
file_handler_list, folder_handler_list = [], []
def enum_folder(parent_folder):
parent_folder.expand(["Files", "Folders"]).get().execute_query()
for file in parent_folder.files: # In the event that the directory ends in a file.
file_handler_list.append(PathHandler(to_map.get_scheme_and_root_from_absolute() + file.serverRelativeUrl))
for folder in parent_folder.folders: # In the event that the directory ends in a folder.
folder_handler_list.append(PathHandler(to_map.get_scheme_and_root_from_absolute() + folder.serverRelativeUrl))
enum_folder(folder)
root_folder = self.client_context.web.get_folder_by_server_relative_url(to_map.get_relative_from_absolute())
enum_folder(root_folder)
print(f"\nMapping complete. {len(file_handler_list)} file/s + {len(folder_handler_list)} folder/s found.")
return file_handler_list, folder_handler_list
The first function create_client_context
creates a ClientContext instance using the user credentials (necessary for accessing the SharePoint site). The second function map_folder
is used to recursively append all PathHandler instances of all files and folders to their own lists.
The second class is PathHandler
, which I've made in an attempt to simplify the way that I make alterations to the urls / local paths as I need them, eg: sometimes I'll need a relative url exclusive of scheme and site name. This class is shown below:
import os
import urllib.parse
from pathlib import Path
class PathHandler(object):
def __init__(self, absolute_path:str) -> None:
self.absolute_path = absolute_path
def get_filename_from_absolute(self) -> str: # COMPLETE ✓
parsed_url = urllib.parse.urlparse(self.absolute_path)
return os.path.basename(parsed_url.path)
def strip_filename(self) -> str: # COMPLETE ✓
return self.absolute_path[:-len(self.get_filename_from_absolute())]
def get_relative_from_absolute(self) -> str: # COMPLETE ✓
parsed_url = urllib.parse.urlparse(self.absolute_path)
return parsed_url.path
def get_parent_folder_from_absolute(self) -> str: # COMPLETE ✓
parsed_url = urllib.parse.urlparse(self.absolute_path)
return os.path.dirname(parsed_url.path)
def get_scheme_and_root_from_absolute(self) -> str: # COMPLETE ✓ - Not to be used for local paths
parsed_url = urllib.parse.urlparse(self.absolute_path)
return f"{parsed_url.scheme}://{parsed_url.netloc}"
def convert_to_absolute_local(self, local_root:str, global_root:str) -> str: # COMPLETE ✓
temporary_path = local_root + self.absolute_path[len(global_root):]
return temporary_path.replace("//", os.sep)
def convert_to_absolute_global(self, local_root:str, global_root:str) -> str: # COMPLETE ✓
return global_root + self.absolute_path[len(local_root):].replace(os.sep, "//")
My question:
The above scripts tie into quite a large project, and I am trying to simplify this by incorporating OOP. However, I've not worked in an OOP manner before, and am concerned that PathHandler
exists unnecessarily. Many of my SharePointHandler
methods can be simplified to return strings, but to remain consistent, I am encouraged to return instances of PathHandler
.
Is this PathHandler
class indeed adding unnecessary complexity to my code? Should I simply remove the class and use the functions as typical helper functions and make the necessary adjustments to the method inputs at the start of a function call?
1 Answer 1
Since you are using type hints you must be using Python 3 (good). So don't inherit from object
.
Ideally all members of a class are first set in the constructor. So make your create_client_context
return a ClientContext
rather than mutating self
.
Your print
s should not exist in create_client_context
. You can keep your try
if you narrow Exception
down to the actual exception type from the client context and rethrow using a custom application domain exception; or else don't try
/except
at all.
Recursing on enum_folder
is risky. It's possible to blow the (fairly shallow) Python stack on a folder nesting level that is too deep.
You might want to use the built-in logger support from map_folder
, but don't print.
You don't use password
outside of create_client_context
, so for safety's sake don't store it on self
.
Move your urllib.parse.urlparse
to a centralised call that occurs in the constructor of your PathHandler
and stores the result on self
, something like
def __init__(self, absolute_path:str) -> None:
self.absolute_path = absolute_path
self.parsed_url = urllib.parse.urlparse(self.absolute_path)
Delete your # COMPLETE ✓
. All code should be considered complete if it doesn't throw. If you want to mark something incomplete, then raise a NotImplementedError
(this is a reasonable way to write out skeletons).
-
\$\begingroup\$ Thank you so much for your detailed answer - so many improvements seems obvious now that they're mentioned. Regarding the mutation of
self
increate_client_context
, would it be as simple as droppingself
as a parameter and having the method return an instance ofClientContext
? \$\endgroup\$ChaddRobertson– ChaddRobertson2022年04月18日 19:56:07 +00:00Commented Apr 18, 2022 at 19:56 -
\$\begingroup\$ As for the potential stack issues with
enum
, your comments are incredibly valid - I've opted for this method because it allows for utility methods to be passed tomap_folder
that use the File and Folder instances as input parameters. This just allows for file operations to be done whilst the folder is being mapped, rather than afterwards. I've done a bit of testing, and so far I've not come close to max depth (never say never). \$\endgroup\$ChaddRobertson– ChaddRobertson2022年04月18日 20:02:49 +00:00Commented Apr 18, 2022 at 20:02 -
\$\begingroup\$ The only improvement I'm confused about is the second-to-last one that you've mentioned. If you have any free time, could you perhaps demonstrate what you mean? I'd really appreciate the insight. \$\endgroup\$ChaddRobertson– ChaddRobertson2022年04月18日 20:04:05 +00:00Commented Apr 18, 2022 at 20:04
-
1\$\begingroup\$ Updated for
parsed_url
\$\endgroup\$Reinderien– Reinderien2022年04月18日 20:19:16 +00:00Commented Apr 18, 2022 at 20:19