MIME-Version: 1.0
In-Reply-To: 
 <CAE+jja=Gdd032H7tpoZD2C0m2R7SnTZpHX_oPx2K2zGbaaW9yg@mail.gmail.com>
References: 
 <CAE+jja=Gdd032H7tpoZD2C0m2R7SnTZpHX_oPx2K2zGbaaW9yg@mail.gmail.com>
From: Dave Page <dpage@pgadmin.org>
Date: Wed, 4 Apr 2018 17:17:01 +0100
Message-ID: 
 <CA+OCxozPLOGjsabKwLHMsCOqCkXcPuZYyqqjgM1atLVQpkdU8w@mail.gmail.com>
Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
To: Joao De Almeida Pereira <jdealmeidapereira@pivotal.io>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>,
	Khushboo Vashi <khushboo.vashi@enterprisedb.com>,
	Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com>
Content-Type: multipart/alternative; boundary="001a1148e3d22ec98905690827b2"
Precedence: bulk

--001a1148e3d22ec98905690827b2
Content-Type: text/plain; charset="UTF-8"

Khushboo, Murtuza,

Can you spend some time reviewing this please? I've started playing with it
as well - the first thing that's irking me somewhat is the lack of
comments. Descriptive function names are all well and good, but sometimes a
little more is needed, especially for less experienced developers or
newcomers to the application!

On Mon, Apr 2, 2018 at 7:45 PM, Joao De Almeida Pereira <
jdealmeidapereira@pivotal.io> wrote:

> Hi Hackers,
>
> Attached you can find the patch that will start to decouple pgAdmin from
> ACITree library.
> This patch is intended to be merged after 3.0, because we do not want to
> cause any entropy or delay the release, but we want to start the discussion
> and show some code.
>
> This job that we started is a massive tech debt chore that will take some
> time to finalize and we would love the help of the community to do it.
>
> *Summary of the patch:*
> 0001 patch:
>  - Creates a new tree that will allow us to create a separation between
> the application and ACI Tree
>  - Creates a Fake Tree (Test double, for reference on the available test
> doubles: https://martinfowler.com/bliki/TestDouble.html) that can be used
> to inplace to replace the ACITree and also encapsulate the new tree
> behavior on our tests
>  - Adds tests for all the tree functionalities
>
> 0002 patch:
>  - Extracts, refactors, adds tests and remove dependency from ACI Tree on:
> - getTreeNodeHierarchy
> - on backup.js: menu_enabled, menu_enabled_server,
> start_backup_global_server, backup_objects
> - on datagrid.js: show_data_grid, get_panel_title, show_query_tool
> - Start using sprintf-js as Underscore.String is deprecating sprintf
> function
>
> This patch represents only 10 calls to ACITree.itemData out of 176 that
> are spread around our code
>
>
> *In Depth look on the process behind the patch:*
>
> We started writing this patch with the idea that we need to decouple
> pgAdmin4 from ACITree, because ACITree is no longer supported, the
> documentation is non existent and ACITree is no longer being actively
> developed.
>
> Our process:
> 1. We "randomly" selected a function that is part of the ACITree. From
> this point we decided to replace that function with our own version. The
> function that we choose was "itemData".
> The function gives us all the "data" that a specific node of the tree find.
> Given in order to replace the tree we would need to have a function that
> would give us the same information. We had 2 options:
>   a) Create a tree with a function called itemData
> Pros:
>  - At first view this was the simpler solution
>  - Would keep the status quo
> Cons:
>  - Not a OOP approach
>  - Not very flexible
>   b) Create a tree that would return a node given an ID and then the node
> would be responsible for giving it's data.
> Pros:
>  - OOP Approach
>  - More flexible and we do not need to bring the tree around, just a node
> Cons:
>  - Break the current status quo
>
> Given these 2 options we decided to go for a more OOP approach creating a
> Tree and a TreeNode classes, that in the future will be renamed to
> ACITreeWrapper and TreeNode.
>
> 2. After we decided on the starting point we searched for occurrences of
> the function "itemData" and we found out that there were 303 occurrences of
> "itemData" in the code and roughly 176 calls to the function itself (some
> of the hits were variable names).
>
> 3. We selected the first file on the search and found the function that
> was responsible for calling the itemData function.
>
> 4. Extracted the function to a separate file
>
> 5. Wrap this function with tests
>
> 6. Refactor the function to ES6, give more declarative names to variables
> and break the functions into smaller chunks
>
> 7. When all the tests were passing we replaced ACITree with our Tree
>
> 8. We ensured that all tests were passing
>
> 9. Remove function from the original file and use the new function
>
> 10. Ensure everything still works
>
> 11. Find the next function and execute from step 4 until all the functions
> are replaced, refactored and tested.
>
> As you can see by the process this is a pretty huge undertake, because of
> the number of calls to the function. This is just the first step on the
> direction of completely isolating the ACITree so that we can solve the
> problem with a large number of elements on the tree.
>
> *What is on our radar that we need to address:*
>  - Finish the complete decoupling of the ACITree
>  - Performance of the current tree implementation
>  - Tweak the naming of the Tree class to explicitly tell us this is to use
> only with ACITree.
>
>
> Thanks
> Joao
>
>


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--001a1148e3d22ec98905690827b2
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Khushboo, Murtuza,<div><br></div><div>Can you spend some t=
ime reviewing this please? I&#39;ve started playing with it as well - the f=
irst thing that&#39;s irking me somewhat is the lack of comments. Descripti=
ve function names are all well and good, but sometimes a little more is nee=
ded, especially for less experienced developers or newcomers to the applica=
tion!</div></div><div class=3D"gmail_extra"><br><div class=3D"gmail_quote">=
On Mon, Apr 2, 2018 at 7:45 PM, Joao De Almeida Pereira <span dir=3D"ltr">&=
lt;<a href=3D"mailto:jdealmeidapereira@pivotal.io" target=3D"_blank">jdealm=
eidapereira@pivotal.io</a>&gt;</span> wrote:<br><blockquote class=3D"gmail_=
quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1=
ex"><div dir=3D"ltr">Hi Hackers,<div><br></div><div>Attached you can find t=
he patch that will start to decouple pgAdmin from ACITree library.</div><di=
v>This patch is intended to be merged after 3.0, because we do not want to =
cause any entropy or delay the release, but we want to start the discussion=
 and show some code.</div><div><br></div><div>This job that we started is a=
 massive tech debt chore that will take some time to finalize and we would =
love the help of the community to do it.</div><div><br></div><div><b>Summar=
y of the patch:</b></div><div>0001 patch:=C2=A0</div><div>=C2=A0- Creates a=
 new tree that will allow us to create a separation between the application=
 and ACI Tree</div><div>=C2=A0- Creates a Fake Tree (Test double, for refer=
ence on the available test doubles:=C2=A0<a href=3D"https://martinfowler.co=
m/bliki/TestDouble.html" target=3D"_blank">https://martinfowler.<wbr>com/bl=
iki/TestDouble.html</a>) that can be used to inplace to replace the ACITree=
 and also encapsulate the new tree behavior on our tests</div><div>=C2=A0- =
Adds tests for all the tree functionalities</div><div><br></div><div>0002 p=
atch:</div><div>=C2=A0- Extracts, refactors, adds tests and remove dependen=
cy from ACI Tree on:</div>   - getTreeNodeHierarchy <br>    - on backup.js:=
 menu_enabled, menu_enabled_server, start_backup_global_server, backup_obje=
cts<br>    - on datagrid.js: show_data_grid, get_panel_title, show_query_to=
ol<div>- Start using sprintf-js as Underscore.String is deprecating sprintf=
 function<br><div>=C2=A0=C2=A0</div><div>This patch represents only 10 call=
s to ACITree.itemData out of 176 that are spread around our code</div><div>=
<br></div><div><b><br></b></div><div><b>In Depth look on the process behind=
 the patch:</b></div><div><br></div><div>We started writing this patch with=
 the idea that we need to decouple pgAdmin4 from ACITree, because ACITree i=
s no longer supported, the documentation is non existent and ACITree is no =
longer being actively developed.</div><div><br></div><div>Our process:</div=
><div>1. We &quot;randomly&quot; selected a function that is part of the AC=
ITree. From this point we decided to replace that function with our own ver=
sion. The function that we choose was &quot;itemData&quot;.</div><div>The f=
unction gives us all the &quot;data&quot; that a specific node of the tree =
find.</div><div>Given in order to replace the tree we would need to have a =
function that would give us the same information. We had 2 options:</div><d=
iv>=C2=A0 a) Create a tree with a function called itemData</div><div>Pros:<=
/div><div>=C2=A0- At first view this was the simpler solution</div>=C2=A0- =
Would keep the status quo<div>Cons:</div><div>=C2=A0- Not a OOP approach</d=
iv><div>=C2=A0- Not very flexible</div><div>=C2=A0 b) Create a tree that wo=
uld return a node given an ID and then the node would be responsible for gi=
ving it&#39;s data.</div><div>Pros:=C2=A0</div><div>=C2=A0- OOP Approach</d=
iv><div>=C2=A0- More flexible and we do not need to bring the tree around, =
just a node</div><div>Cons:</div><div>=C2=A0- Break the current status quo<=
/div><div><br></div><div>Given these 2 options we decided to go for a more =
OOP approach creating a Tree and a TreeNode classes, that in the future wil=
l be renamed to ACITreeWrapper and TreeNode.</div><div><br></div><div>2. Af=
ter we decided on the starting point we searched for occurrences of the fun=
ction &quot;itemData&quot; and we found out that there were 303 occurrences=
 of &quot;itemData&quot; in the code and roughly 176 calls to the function =
itself (some of the hits were variable names).</div><div><br></div><div>3. =
We selected the first file on the search and found the function that was re=
sponsible for calling the itemData function.</div><div><br></div><div>4. Ex=
tracted the function to a separate file</div><div><br></div><div>5. Wrap th=
is function with tests</div><div><br></div><div>6. Refactor the function to=
 ES6, give more declarative names to variables and break the functions into=
 smaller chunks</div><div><br></div><div>7. When all the tests were passing=
 we replaced ACITree with our Tree</div><div><br></div><div>8. We ensured t=
hat all tests were passing</div><div><br></div><div>9. Remove function from=
 the original file and use the new function</div><div><br></div><div>10. En=
sure everything still works</div><div><br></div><div>11. Find the next func=
tion and execute from step 4 until all the functions are replaced, refactor=
ed and tested.</div><div><br></div><div>As you can see by the process this =
is a pretty huge undertake, because of the number of calls to the function.=
 This is just the first step on the direction of completely isolating the A=
CITree so that we can solve the problem with a large number of elements on =
the tree.</div><div><br></div><div><b>What is on our radar that we need to =
address:</b></div><div>=C2=A0- Finish the complete decoupling of the ACITre=
e</div><div>=C2=A0- Performance of the current tree implementation</div><di=
v>=C2=A0- Tweak the naming of the Tree class to explicitly tell us this is =
to use only with ACITree.</div><div><br></div><div><br></div><div>Thanks</d=
iv><span class=3D"HOEnZb"><font color=3D"#888888"><div>Joao</div><div><br><=
/div></font></span></div></div>
</blockquote></div><br><br clear=3D"all"><div><br></div>-- <br><div class=
=3D"gmail_signature" data-smartmail=3D"gmail_signature">Dave Page<br>Blog: =
<a href=3D"http://pgsnake.blogspot.com" target=3D"_blank">http://pgsnake.bl=
ogspot.com</a><br>Twitter: @pgsnake<br><br>EnterpriseDB UK: <a href=3D"http=
://www.enterprisedb.com" target=3D"_blank">http://www.enterprisedb.com</a><=
br>The Enterprise PostgreSQL Company<br></div>
</div>

--001a1148e3d22ec98905690827b2--