Refactoring: From quick and dirty to pythonic II

Refactoring: From quick and dirty to pythonic (Part II)

This blog post is the second part in a three part series that describes my efforts in refactoring a script into a better state.
In the first part of this series I analyzed the shortcomings of the initial script and outlined the refactoring to apply. These were:

• Decompose lengthy methods using Extract method to reduce complexity and increase readability
• Decompose Conditional to likewise reduce complexity and make the code more readable.
• Introduction of the Builder pattern in order to better structure the code and group related responsibilities using a well known pattern.\

Let’s look at how this worked out.

Decompose methods

Decomposing large and lengthy methods is easy to do thanks to wide spread IDE support. In IntelliJ for example you just have to highlight the part of the method to extract and press CRTL+ALT+M.
But beware that just extracting methods in order to decrease the length of the original methods need not necessarily increases readability. The key points to key in mind when extracting methods are:

• The extracted part should contain related code that concerns a specific responsibility
• Avoid introducing lengthy parameter lists as this would introduce another code smell
• Use meaningful names for methods and parameters. This obviously makes the code easier to understand

In an ideal case the extracted methods appear in the code as they are called from the original methos:

def method_a():
    method_b()
    method_c()
    method_d()

def method_b()
    # Do something meaningful here
    pass

def method_c()
    # Do something meaningful here
    pass

def method_d()
    # Do something meaningful here
    pass

An example of how that worked for the script can be seen in lines 88 to 115 of the new version of the script where the original method get_helm_releases has been broken up into several smaller methods each having a single responsibility*.

*In addition enforced by encapsulating the code in a separate class

Introduction of a Builder

Most of code in the scripts concerns the construction of objects from yaml. Thus it seemed natural to apply one of the creational patterns and the builder pattern seemed to fit quiet nicely.
Instead of implementing the pattern in a whole -including it’s type hierarchy-, I opted for a more light weight approach with classes implementing the building logic in a common method build(). Because of pythons dynamic type checking nature the builder instances can nevertheless be used in a generic way (see below ) to build objects.
I created such a builder class for each type to build (GitRepository, HelmRelease and ConfigMap).

Code for building the objects

Building the objects from the yaml files can now be done using a generic function thanks to some syntactical sugar of python:

def create_from_files(folder: str, builder_class, *builder_init_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 = builder_class(yaml_doc, *builder_init_args)
                created_object = builder.build()
                if created_object:
                    created_objects[created_object.name] = created_object
    return created_objects
...

helm_releases = create_from_files(path_to_helm_releases, HelmReleaseBuilder, repos, values)

The function take a folder containing files (folder), the class of the builder (builder_class) and the constructor args for the builders constructor (builder_init_args).
Each yaml block contained in one of the file is used to create an instance of the builder class and to invoke its build-method. If the builder was able to successfully create an object it is added to a map of created objects. The invocation for helm releases is shown as the last line in the snippet.
The neat thing about this function is that it can be used for any type to create. The downside of this approach however is that the contract the builder class has to fullfil (Logic of the build method, first argument of constructor is the yaml block) is not made explicit but only becomes apparent when knowing the workings of the method.Weighing the addition of yet more code against the need for being more explicit, I decided to keep the code as it is.

Analysis of the new version

Let’s look at the second, refactored version of the script to see whether the refactorings did indeed resolved the codes shortcomings:

• Long methods and Spaghetti code: Except for the main method which still needs refactoring, the length of the methods has been reduced significantly and all of them are more readable.
• Nested loops and if statements: Loops have been decomposed by extracting the body of a loop into meaningfully named methods. Complex or lengthy if statements have been extracted into separate methods. Both times the Extract Method refactoring was applied. The result is that the code is more readable. This refactoring also contributed to the reduction of length and complexity of the methods.
• Copied and pasted code: The repetitive code used for looping of the files has been replaced with a function that can now be used generically.

Other smaller improvements include:

• The introduction of the builder classes now neatly groups related code
• The utility method find makes querying the yaml objects nicely readable: name=find("metadata/name", self.yaml_doc)

Conclusion

The refactored version of the script is certainly better than the first version. Improvements include:

• Increased Readability
• Reduced Complexity
• Introduction of a standard pattern
• Grouping of related code

But these refactorings also introduced some new issues:

• Source code size increased
• Builders introduced a significant amount of boiler plate code not related to the actual function of the script

In addition the oo/functional mixture is still there and the script is not yet as pythonic as it could be. So I decided to give the script another try to solve these issues. And if you want to see me making a maybe surprising flip backwards, I invite you to the last part of this series where I move from oo back to functional in an effort to make it look like I have been programming python for decades ;-)

Back to Home