On Mon, Apr 30, 2018 at 11:27 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote:
> You've lost us with the last reply. We'd love to know what we'd have to do > to get this patch committed. You mention that "some things are missing at > some places" and that "there are missing bits". > I've already mentioned few of them in the earlier mails about them. > Please note that this is not a complete solution that we've offered so far. > I know and, hence - asked for your understanding of the code (and, asked for the TODOs.) > But only one step in a grander effort to effect a more cleaner, more > maintainable, more testable, code base. > Yes - we all want cleaner, maintainable, and testable code base. As per my understading comments are required for better understanding, and makes the code more maintainable. But - if we're going to change everything in one go, it is difficult to understand the changes. Let's avoid mix match multiple things in a single patch. And, concerntrate one functionality at a time. -- Thanks, Ashesh > > Thanks > Joao and Anthony. > > On Mon, Apr 30, 2018 at 11:45 AM Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> On Mon, Apr 30, 2018 at 9:07 PM, Dave Page <dp...@pgadmin.org> wrote: >> >>> >>> >>> On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi < >>> ashesh.va...@enterprisedb.com> wrote: >>> >>>> >>>> On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aeme...@pivotal.io> >>>> wrote: >>>> >>>>> I was expecting a separate layer between the tree implementation, and >>>>>> aciTree adaptor. >>>>>> Please find the patch for the example. >>>>>> It will separate the two layers, and easy to replace with the new >>>>>> implementation in future. >>>>> >>>>> >>>>> In general, we want defer the separation of the layers for now. Even >>>>> though we might assume that this is the direction we want to go in. It's >>>>> simply too early to be making such an architectural leap. For right now, >>>>> we >>>>> just know that we need the decoupling, but don't know what if we'd need >>>>> the >>>>> 2 layers *as implemented*. The principle we're adhering to here is >>>>> the Last Responsible Moment principle, which states that you only make the >>>>> changes that you feel is necessary for the given problems you're facing: >>>>> https://blog.codinghorror.com/the-last-responsible-moment/ >>>>> >>>>> I would not like to see that changes in this patch. >>>>>> I would like us to come up with the actual design about the hot >>>>>> pluggability before going in this direction. >>>>> >>>>> >>>>> In our point of view these 2 changes are not related. One thing is the >>>>> internal code organization of the application, other thing is allowing >>>>> third party to drop code in the application and it just work. These 2 >>>>> should be talked separately, but the hot pluggability is not something >>>>> that >>>>> will be address by this work we are doing right now. >>>>> >>>> Neither - it should be part of this change. >>>> It should be addressed separately, and discussed. >>>> >>> >>> I agree. As long as this work doesn't make the pluggability problem >>> worse, that problem should be considered separately. >>> >>> So given Anthony's comments, are you happy with this patch? >>> >> I liked the design so far. >> But - as Khushboo mentioned ealier - it is missing at some places. >> I had to read through the code to understand the execution flow for some. >> >> And, there is still a lot missing bits, and pieces to consider for commit >> in the repo. >> >> -- Thanks, Ashesh >> >>> >>> >>>> >>>> -- Thanks, Ashesh >>>> >>>>> >>>>> Anthony && Joao >>>>> >>>>> On Mon, Apr 30, 2018 at 11:03 AM, Ashesh Vashi < >>>>> ashesh.va...@enterprisedb.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi < >>>>>> ashesh.va...@enterprisedb.com> wrote: >>>>>> >>>>>>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira < >>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>> >>>>>>>> Hi Hackers, >>>>>>>> As you are aware we kept on working on the patch, so we are >>>>>>>> attaching to this email a new version of the patch. >>>>>>>> This new version contains all the changes in the previous one plus >>>>>>>> more extractions of functions and refactoring of code. >>>>>>>> >>>>>>>> The objective of this patch is to create a separation between >>>>>>>> pgAdmin and the ACI Tree. We are doing this because we realized that at >>>>>>>> this point in time we have the ACI Tree all over the code of pgAdmin. I >>>>>>>> found a very interesting article that really talks about this: >>>>>>>> https://medium.freecodecamp.org/code-dependencies-are-the- >>>>>>>> devil-35ed28b556d >>>>>>>> >>>>>>>> In this patch there are some visions and ideas about the location >>>>>>>> of the code, the way to organize it and also try to pave the future >>>>>>>> for a >>>>>>>> application that is stable, easy to develop on and that can be release >>>>>>>> at a >>>>>>>> times notice. >>>>>>>> >>>>>>>> We are investing a big chunk of our time in doing this refactoring, >>>>>>>> but while doing that we also try to respond to the patches sent to the >>>>>>>> mailing list. We would like the feedback from the community because we >>>>>>>> believe this is a changing point for the application. The idea is to >>>>>>>> change >>>>>>>> the way we develop this application, instead of only correcting a bug >>>>>>>> of >>>>>>>> developing a feature, with every commit we should correct the bug or >>>>>>>> develop a feature but leave the code a little better than we found it >>>>>>>> (Refactoring, refactoring, refactoring). This is hard work but that is >>>>>>>> what >>>>>>>> the users from pgAdmin expect from this community of developers. >>>>>>>> >>>>>>>> >>>>>>>> ====== >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> It is a huge patch >>>>>>>> 86 files changed, 5492 inserts, 1840 >>>>>>>> deletions >>>>>>>> and we would like to get your feedback as soon as possible, because >>>>>>>> we are continuing to work on it which means it is going to grow in >>>>>>>> size. >>>>>>>> >>>>>>>> >>>>>>>> At this point in time we still have 124 of 176 calls to the >>>>>>>> function itemData from ACITree. >>>>>>>> >>>>>>>> What does each patch contain: >>>>>>>> 0001: >>>>>>>> Very simple patch, we found out that the linter was not looking >>>>>>>> into all the javascript test files, so this patch will ensure it is >>>>>>>> >>>>>>> Committed the patch along with the regression introduced because of >>>>>>> this patch. >>>>>>> >>>>>>>> >>>>>>>> 0002: >>>>>>>> New Tree abstraction. This patch contains the new Tree that works >>>>>>>> as an adaptor for ACI Tree and is going to be used on all the >>>>>>>> extractions >>>>>>>> that we are doing. >>>>>>>> >>>>>>> >>>>>>> I was expecting a separate layer between the tree implementation, >>>>>>> and aciTree adaptor. >>>>>>> Please find the patch for the example. >>>>>>> >>>>>>> It will separate the two layers, and easy to replace with the new >>>>>>> implemenation in future. >>>>>>> >>>>>> >>>>>> Oops forgot to attach the patch. >>>>>> Please find the patch attached. >>>>>> >>>>>> -- Thanks, Ashesh >>>>>> >>>>>>> >>>>>>>> 0003: >>>>>>>> Code that extracts, wrap with tests and replace ACI Tree >>>>>>>> invocations. >>>>>>>> >>>>>>> There are many small cases left in the patches. >>>>>>> Hence - I would like to know the TODO list created by you. >>>>>>> >>>>>>> e.g. When we remove any of the object from the database server, >>>>>>> we're not yet removing the respective node from the new implementation, >>>>>>> and >>>>>>> its children. >>>>>>> >>>>>>>> >>>>>>>> We start creating new pattern for the location of Javascript >>>>>>>> files and their structure. >>>>>>>> >>>>>>> I would not like to see that changes in this patch. >>>>>>> I would like us to come up with the actual design about the hot >>>>>>> pluggability before going in this direction. >>>>>>> >>>>>>>> Create patterns for creation of dialogs (backup and restore) >>>>>>>> >>>>>>> It's better - we don't change the directory structure at the moment. >>>>>>> >>>>>>> I am not against dividing the big javascript files in small chunks, >>>>>>> but - I would like us to discuss first about the hot plugins design >>>>>>> first. >>>>>>> >>>>>>> -- Thanks, Ashesh >>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Thanks >>>>>>>> Joao >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi < >>>>>>>> ashesh.va...@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> I have quite a few comments for the patch. >>>>>>>>> I will send them soon. >>>>>>>>> >>>>>>>>> On Fri, Apr 27, 2018, 14:45 Dave Page <dp...@pgadmin.org> wrote: >>>>>>>>> >>>>>>>>>> How is your work on this going Ashesh? Will you be committing >>>>>>>>>> today? >>>>>>>>>> >>>>>>>>>> On Wed, Apr 25, 2018 at 8:52 AM, Dave Page <dp...@pgadmin.org> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Ashesh; you had agreed to work on this early this week. Please >>>>>>>>>>> ensure you do so today. >>>>>>>>>>> >>>>>>>>>>> Thanks. >>>>>>>>>>> >>>>>>>>>>> On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira < >>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi Hackers, >>>>>>>>>>>> >>>>>>>>>>>> Can someone review and merge this patch? >>>>>>>>>>>> >>>>>>>>>>>> Thanks >>>>>>>>>>>> Joao >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira < >>>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi Hackers, >>>>>>>>>>>>> Any other comment about this patch? >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks >>>>>>>>>>>>> Joao >>>>>>>>>>>>> >>>>>>>>>>>>> On Tue, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira < >>>>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hello Khushboo >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi < >>>>>>>>>>>>>> khushboo.va...@enterprisedb.com> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi Joao, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I have reviewed your patch and have some suggestions. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira < >>>>>>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Hello Murtuza/Dave, >>>>>>>>>>>>>>>> Yes now the extracted functions are spread into different >>>>>>>>>>>>>>>> files. The intent would be to make the files as small as >>>>>>>>>>>>>>>> possible, and also >>>>>>>>>>>>>>>> to group and name them in a way that would be easy to >>>>>>>>>>>>>>>> understand what each >>>>>>>>>>>>>>>> file is doing without the need of opening it. >>>>>>>>>>>>>>>> As a example: >>>>>>>>>>>>>>>> static/js/backup will contain all the backup related >>>>>>>>>>>>>>>> functionality inside of this folder we can see the file: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> menu_utils.js At this moment in time we decided to group >>>>>>>>>>>>>>>> all the functions that are related to the menu, but we can >>>>>>>>>>>>>>>> split that also >>>>>>>>>>>>>>>> if we believe it is easier to see. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> It's really very good to see the separated code for backup >>>>>>>>>>>>>>> module. As we have done for backup, we would like do it for >>>>>>>>>>>>>>> other PG >>>>>>>>>>>>>>> utilities like restore, maintenance etc. >>>>>>>>>>>>>>> Considering this, we should separate the code in a way that >>>>>>>>>>>>>>> some of the common functionalities can be used for other >>>>>>>>>>>>>>> modules like menu >>>>>>>>>>>>>>> (as you have mentioned above), dialogue factory etc. >>>>>>>>>>>>>>> Also, I think these functionalities should be in their >>>>>>>>>>>>>>> respective static folder instead of pgadmin/static. >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> About the location of the files. The move of the files to >>>>>>>>>>>>>> pgadmin/static/js was made on purpose in order to clearly >>>>>>>>>>>>>> separate >>>>>>>>>>>>>> Javascript from python code. >>>>>>>>>>>>>> The rational behind it was >>>>>>>>>>>>>> - Create a clear separation between the backend and frontend >>>>>>>>>>>>>> - Having Javascript code concentrated in a single place, >>>>>>>>>>>>>> hopefully, will encourage to developers to look for a >>>>>>>>>>>>>> functionality, that >>>>>>>>>>>>>> is already implemented in another modules, because they are >>>>>>>>>>>>>> right there. >>>>>>>>>>>>>> (When we started this journey we realized that the 'nodes' have >>>>>>>>>>>>>> a big >>>>>>>>>>>>>> groups of code that could be shared, but because the Javascript >>>>>>>>>>>>>> is spread >>>>>>>>>>>>>> everywhere it is much harder to look for it) >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> There are some drawbacks of this separation: >>>>>>>>>>>>>> - When creating a new module we will need to put the >>>>>>>>>>>>>> javascript in a separate location from the backend code >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> static/js/datagrid folder contains all the datagrid >>>>>>>>>>>>>>>> related functionality >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Same as backup module, this should be in it's respective >>>>>>>>>>>>>>> static/js folder. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Inside of the folder we can see the files: >>>>>>>>>>>>>>>> get_panel_title.js is responsible for retrieving the name >>>>>>>>>>>>>>>> of the panel >>>>>>>>>>>>>>>> show_data.js is responsible for showing the datagrid >>>>>>>>>>>>>>>> show_query_tool.js is responsible for showing the query >>>>>>>>>>>>>>>> tool >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Does this structure make sense? >>>>>>>>>>>>>>>> Can you give an example of a comment that you think is >>>>>>>>>>>>>>>> missing and that could help? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> As a personal note, unless the algorithm is very obscure or >>>>>>>>>>>>>>>> very complicated, I believe that if the code needs comments it >>>>>>>>>>>>>>>> is a signal >>>>>>>>>>>>>>>> that something needs to change in terms of naming, structure >>>>>>>>>>>>>>>> of the part in >>>>>>>>>>>>>>>> question. This being said, I am open to add some comments that >>>>>>>>>>>>>>>> might help >>>>>>>>>>>>>>>> people. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> You are right, with the help of naming convention and >>>>>>>>>>>>>>> structure of the code, any one can get the idea about the code. >>>>>>>>>>>>>>> But it is >>>>>>>>>>>>>>> very useful to understand the code >>>>>>>>>>>>>>> very easily with the proper comments especially when there >>>>>>>>>>>>>>> are multiple developers working on a single project. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I found some of the places where it would be great to have >>>>>>>>>>>>>>> comments. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> - treeMenu: new tree.Tree() in a browser.js >>>>>>>>>>>>>>> - tree.js (especially Tree class) >>>>>>>>>>>>>>> >>>>>>>>>>>>>> About the comment point I need a more clear understanding on >>>>>>>>>>>>>> what kind of comments you are looking for. Because when you read >>>>>>>>>>>>>> the >>>>>>>>>>>>>> function names you understand the intent, what they are doing. >>>>>>>>>>>>>> The >>>>>>>>>>>>>> parameters also explain what you need to pass into them. >>>>>>>>>>>>>> >>>>>>>>>>>>>> If what you are looking for in these comments is the >>>>>>>>>>>>>> reasoning being the change itself, then that should be present >>>>>>>>>>>>>> in the >>>>>>>>>>>>>> commit message. Specially because this is going to be a very big >>>>>>>>>>>>>> patch with >>>>>>>>>>>>>> a very big number of changes. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks >>>>>>>>>>>>>>>> Joao >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>> Khushboo >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala < >>>>>>>>>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hi Joao, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Patch looks good and working as expected. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I also agree with Dave, Can we please add some comments in >>>>>>>>>>>>>>>>> each file which can help us to understand the flow, I'm >>>>>>>>>>>>>>>>> saying because now >>>>>>>>>>>>>>>>> the code is segregated in so many separate files it will be >>>>>>>>>>>>>>>>> hard to keep >>>>>>>>>>>>>>>>> track of the flow from one file to another when debugging. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>> Murtuza Zabuawala >>>>>>>>>>>>>>>>> EnterpriseDB: http://www.enterprisedb.com >>>>>>>>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Thu, Apr 5, 2018 at 7:08 PM, Joao De Almeida Pereira < >>>>>>>>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hi Khushboo, >>>>>>>>>>>>>>>>>> Attached you can find both patches rebased >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thanks >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi < >>>>>>>>>>>>>>>>>> khushboo.va...@enterprisedb.com> wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Hi Joao, >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Can you please rebase the second patch? >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>>>>> Khushboo >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira >>>>>>>>>>>>>>>>>>> <jdealmeidapere...@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 >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Dave Page >>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>> Twitter: @pgsnake >>>>>>>>>> >>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >>
