Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry <vhenry(at)pivotal(dot)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