Skip to content

Conversation

spookylukey
Copy link
Contributor

This follows the motto "a class with only one method that actually does anything should be a function", with the result that using the director function requires one line of code instead of 5.

This follows the motto "a class with only one method that actually
does anything should be a function", with the result that using
the director function requires one line of code instead of 5.
@codecov-io
Copy link

Codecov Report

Merging #216 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   97.04%   97.02%   -0.02%     
==========================================
  Files          61       61              
  Lines        2303     2288      -15     
==========================================
- Hits         2235     2220      -15     
  Misses         68       68
Impacted Files Coverage Δ
creational/builder.py 94.44% <100%> (-1.12%) ⬇️
tests/test_builder.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e05ca1...1d201b3. Read the comment docs.

@pylang
Copy link

pylang commented Feb 3, 2018

I think this is a good demonstration of how Python can simplify complex concepts from classes to mere functions. However, I see the class implementation as a template. For instance, regarding the present class-based version:

  1. someone might adopt this pattern with many methods in mind, but they may not know how to start
  2. someone from another class-based language (e.g. Java) many wish to compare this pattern in Python
  3. it is presently congruent with other implementation in python-patterns

I see value in both, and so I suggest keeping both the class and function implementations. Perhaps add a comment that the class pattern can be simplified and then implement the function version.

I'd like to see other simplifications in this way for other patterns too. That would be a great way to teach these patterns more succinctly.

@faif
Copy link
Owner

faif commented Feb 3, 2018

I find the points of @pylang fair so I suggest to keep both implementations. @spookylukey would you mind keeping both implementations in the same file and showing how one can use either of them?

@datamafia
Copy link

As someone who watches this repo and enjoys the content, academic nature, and discussion I must say that having a function and class based example is ideal. I appreciate the work.

@spookylukey
Copy link
Contributor Author

My opinion is that I don't see value in having the class version of Director, for the following reasons:

  1. The point of the builder pattern is that the order/process for building is fixed at a high level, and customized at the lower level by implementing varying methods on the builder classes. Making the director a class rather than a function obscures this, as if it might be customized by subclassing.

  2. If you have a more complex building process that needs breaking down into steps, use multiple functions - there is no need to make them into a class with multiple methods.

  3. Providing it as a "template" encourages copy paste coding, resulting in bad APIs

  4. People attracted to this kind of repo (e.g. googling for "design patterns in Python") very often come from a Java background or similar ("design patterns" as a "thing" tend to be less popular with more mature Python developers, because they see most of the famous "GoF" style patterns to be "invisible or simpler" in Python (see Design Patterns in Dynamic Languages ). Such people should be pushed away from the familiarity of what they know to the right solution, otherwise they will write Java in Python, getting the worst of both - Java's verbosity, and the lack of type safety of Python.

From every angle, the right solution is the function. "Simple is better than complex" means we should choose functions. The implementation is shorter for the function. And just look at the API for clients:

Class based:

director = Director()
director.builder = BuilderHouse()
director.construct_building()
building = director.get_building()

Function based:

building = construct_building(BuilderHouse())

And the performance is worse for classes too - we are creating a Director object, only to throw it away a few lines later, plus we have the problem that we might accidentally re-use it (will that work? correctly?).

In fact, I have never needed the builder pattern at all in Python, because there is a simpler way to implement it still. It looks like this:

class Building:
    def __init__(self):
        self.build_foor()
        self.build_size()

    def build_foor(self):
        raise NotImplementedError()

    def build_size(self):
        raise NotImplementedError()

class House(Building):
    def build_floor(self):
        self.floor = "One"

    def build_size(self):
        self.size = "Big"

# etc.

(This was going to be my next PR, as an additional option to what I did before).

This doesn't use an extra external function or class to do the building. Plus, the internals of how Building is made are now logically within Building, rather than being external, which is the way we expect things to be in Python. We have achieved the desired aim of "decoupling the creation of a complex object and its representation" - the Building constructor working entirely on an abstract level, the subclasses work in the detail.

Why is this not used in GoF/Java land?

AFAICS the real reason is how C++ works. If you attempt the above in C++, it doesn't work. That is because in C++ when a superclass constructor (e.g. Building) is executed, the vtable for that object has not been fully constructed - it has only been constructed as far as Building (i.e. incuding Building's superclasses, if any, but not including any subclasses e.g. House). This means that 'downwards' virtual calls do not work in a constructor context (see https://stackoverflow.com/questions/1453131/how-can-i-get-polymorphic-behavior-in-a-c-constructor ).

Another possible reason for this pattern's popularity in Java is that you might have other classes that want the same constructor behaviour as House and Flat, but have some different base class for some reason (seems a bit unlikely but possible I guess). In that case, in Python, we can just make our Building into BuildingMixin and do multiple inheritance, which you can't do in Java.

In other words, the proliferation of the 'Builder' pattern at all is inherited legacy from C++ and Java, now being dragged into Python due to "just copy-and-paste this template" programming.

How do we stop this? By implementing everything as simply as possible. Sometimes a really complex object might benefit from being having a constructor that is outside the class itself, so I can see some value in having a separate function or mixin for it, but that is a simple refactoring you can do when you need to, and doesn't justify its own pattern. Patterns are basically there as common solutions to common problems, but we don't need to solve C++/Java problems that don't exist in Python.

If you would like, I'm happy to write all this up into a new PR, in a much simplified form, explaining what I think would be the best practice, and (very briefly) why more complex solutions are needed. This would then include some of the discussion that @datamafia would like to see. I would mention the alternative solutions, or link to them, and why I think they are unnecessary in Python. From my perspective that kind of thing would make this repo really excellent. I often say "most GoF design patterns are invisible or simpler in Python" (quoting Norvig), but I don't have a good place to point people to in order to demonstrate this. I realise this might require a fair number of changes (especially to creational patterns which are usually massively simpler in Python due to dynamic typing and first class types), but it would be great if this repo could be such a thing.

@faif
Copy link
Owner

faif commented Feb 5, 2018

I like the idea of this repo having the simplest and most Pythonic solution for every pattern, so I propose to go that way. In that sense it would be better to replace this example with the very last one that you propose @spookylukey (Building, House) to show how an explicit builder is not needed. If something is invisible or simpler in Python then we need to show how.

@pylang
Copy link

pylang commented Feb 5, 2018

+1 for implementing the simple version of the class. I would still include the function variant for reasons previously mentioned.

@spookylukey
Copy link
Contributor Author

spookylukey commented Feb 6, 2018

BTW, I should add - I apologize for being so opinionated. I'm afraid I am opinionated, I sometimes try to pretend not to be but that clearly doesn't work. I try harder not to express myself too arrogantly, but usually fail at that too. Thank you for overlooking these faults!

@faif faif merged commit e9a08d0 into faif:master Feb 6, 2018
@faif
Copy link
Owner

faif commented Feb 6, 2018

@spookylukey 👍 No worries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants