Refactoring: From quick and dirty to pythonic III - Wed, Jun 1, 2022
Refactoring: From quick and dirty to pythonic III
Refactoring: From quick and dirty to pythonic (Part III)
This blog post is the third and final part in a three part series that describes my efforts in refactoring a script into a better state.
In the second part
of this series I started doing some basic refactorings on the initial script
by introducing some more oo style programming. The refactored version of the script
. After examining this new version I found that although generally more readable and better structured, the refactoring also contributed significantly to the overall length of the script and contained a lot of code not really related to solving the initial problem. Especially the OO programming parts.
So I decided to make an effort to shorten the overall script and focus the code on solving the problem at hand rather than fostering a particular programming technique.
Let’s look at how this worked out.
Changes / refactorings
Looking at the stats of the files the objective of less code seems to be fullfil since the new version has 168
lines instead of 185
before. That is even more notable since new functionality has been added.
Code-wise, one of main differences between the two versions is that the Builder classes
have been removed in favor of simple python functions. The reason I did that is because of
all the additional code introduced by these class structures. This more lenient approach with dedicated function seems more readable.
Another big change was to rework the function create_from_files
. Instead of trying to create a single flux object from a file, the method create_flux_objects_from_files
and create_flux_objects_from_yaml_doc
now also can create multiple flux objects from a file, if the file contains multiple yaml blocks separated by ---
.:
Old code:
def create_from_files(folder: str, clazz, *args) -> dict:
created_objects = {}
yaml_files = glob.glob(f"{folder}/*.yaml")
for yaml_file in yaml_files:
with open(yaml_file, 'r') as file_content:
yaml_docs = yaml.load_all(file_content, Loader=yaml.FullLoader)
for yaml_doc in yaml_docs:
builder = clazz(yaml_doc, *args)
created_object = builder.build()
if created_object:
created_objects[created_object.name] = created_object
return created_objects
New code:
def create_flux_objects_from_files(glob_pattern) -> Dict[str, object]:
created_objects = {}
for file in glob.glob(glob_pattern):
with open(file, 'r') as file_stream:
yaml_docs = yaml.load_all(file_stream, Loader=yaml.FullLoader)
create_flux_objects_from_yaml_doc(created_objects, yaml_docs)
return created_objects
def create_flux_objects_from_yaml_doc(created_objects, yaml_docs):
for yaml_doc in yaml_docs:
if not (kind := find("kind", yaml_doc)):
print(f"Could not determine kind from {yaml_doc!s:200.200}...")
elif kind not in Kind2Builder.keys():
print(f"Could not find builder for kind {kind} in {yaml_doc!s:200.200}...")
else:
create_flux_object_from_yaml_doc(created_objects, kind, yaml_doc)
def create_flux_object_from_yaml_doc(created_objects, kind, yaml_doc):
builder = Kind2Builder[kind]
if not (flux_object := builder(yaml_doc)):
print(f"Could not build flux object from {yaml_doc!s:200.200}...")
else:
created_objects[str(flux_object)] = flux_object
The map Kind2Builder
not shown hold the mapping from kind of object to the function to build the object. And although the new code has approximately twice as much lines as the old code, it is nevertheless more readable than the old one. That is because the refactoring Extract method
almost always results in more lines of code than before due to the additional code introduced by the method signature.
Nevertheless is this refactoring a good example on how to decrease nesting
and being more expressive].
In the old version of the script the composition of the helm release was done in the builder and the main method. In the new version I moved that code to a dedicated method:
def compose_helm_releases(flux_objects):
for release in {name: flux_object for name, flux_object in flux_objects.items() if
isinstance(flux_object, HelmRelease)}.values(): # type: HelmRelease
release.repo = flux_objects[GitRepository.__name__ + "/" + release.repo_name]
release.values = flux_objects[HelmConfigValues.__name__ + "/" + release.values_config_map_name]
yield release
That way the code for that task is no longer distributed among methods but in a single place, thus enforcing the single responsibility principle .
But is it more pythonic
Well, looking at a definition of pythonic I have to admit that the code looks definitely more pythonic than the first code. But there is no doubt that it can be made more pythonic. There are idioms like unpacking or slicing which I just did not find a use for and a more experienced python programmer would have found. So still more work to do on that script.
Conclusion
The code of the first version of this script (see first part of this series) really had some bad code smells. The first refactoring attempt made it look better but had an unpleasant mixture of oo code and functional programming. This final attempt shifted the code more to functional programming and made the code the most pythonic or pathetic, whatever you like more ;-)