On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry <vhe...@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. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
