On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry <vhenry@pivotal.io> wrote:
Hi,
Khushboo, thanks for making some of the changes we mentioned in our previous email.
But when you say that they share logic and the code is
if self.node_type == 'procedure':
50+ lines of code
else:
50+ lines of code
This tells us that they share some logic but there is a huge difference between them. This breaks the Single Responsibility Principle, that says that each class should have only one responsibility on the context of the application.
The responsibility here is to handle the objects from pg_proc which are fundamentally almost identical bar a couple of nuances in the SQL spec. Looking at the code you quote (or at least that I think you're quoting), I see there's maybe duplication of 50% - 75% or so of the code in each branch which probably could be cleaned up.
Even if we completely ignore SOLID principles the clear opportunity for a refactor that would make the code much easier to read was lost, when last Monday we agreed that we should not lose these opportunities.
Don't mistake not spotting an opportunity with intentionally ignoring it.
Dave, we don't understand why this change was committed into master, when there was a clear disagreement.
I must be missing an email then. I only saw one from you previously, to which Khushboo addressed all the concerns you raised.
