Re: [pgAdmin4][RM#3235] Code refactoring in Query tool

Hi,

Thank you Victoria & Joao for reviewing.
PFA updated patch.

On Wed, Apr 4, 2018 at 12:26 AM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> Hi Murtuza
>
> It is really good to see other patches that are just refactoring code.
>
> We have some suggestions:
> - We are trying to use === instead of == because === ensure that the type
> is also checked (https://developer.mozilla.org/en-US/docs/Web/JavaScript/
> Equality_comparisons_and_sameness)
>
​Done​

- Now that we are refactoring some code, maybe we should keep some
> consistency on the way we name functions and variables.
>
We should use camelCase for names instead of snake_case. In general, if you
> see a function or variable name that doesn't fit the desired syntax or if a
> block of code seems confusing, it is better to refactor it.
>
Done​, I have also changed other variables.​
BTW I'm using camelCase convention from last few patches :)

- By the name of the function is_new_transaction_required, it describes
> what the function represents but doesn't seem to explain the full scope of
> the function. What do you think about the name: httpResponseRequiresNewT
> ransaction
>
​Done​

- The extraction doesn't look like it matches the code removed
>

> - if (pgAdmin.Browser.UserManagement.is_pga_login_required(e))
> {
> - self.save_state('_explain_timing', []);
> - return pgAdmin.Browser.UserManagement.pga_login();
> - }
> -
> - if(transaction.is_new_transaction_required(e)) {
> - self.save_state('_explain_timing', []);
> - return self.init_transaction();
> - }
> -
> - alertify.alert(gettext('Explain options error'),
> - gettext('Error occurred while setting timing option in
> explain.')
> + let msg = httpErrorHandler.handleQueryToolAjaxError(
> + pgAdmin, self, e, '_explain_timing', null, true
> );
> + alertify.alert(gettext('Explain options error'), msg);
> In this case we are only saving state if the following conditions are
> true:
> isNotConnectedToThePythonServer and httpResponseJSONIsPresent and
> connectionLostToPostgresDatabase and shouldSaveState
> That is not the case on the removed code.
>
​I think the *null* value got your attention b
ut actually I have a check in ​*handleQueryToolAjaxError *which will make
it array [] with respect to arguments for the state to save, Anyways I have
also changed it to pass [] instead of null for better clarity.
We have all those checks in our function so it check for those condition
and save the state only if those returns True.

- The functions extracted when are called do not use all the parameters.
> This tells us that the function groups too much functionality that is not
> used in same cases. Maybe we should split this function into smaller
> functions that do each part.
>
​We already had split up the function in two part.
​

>
>
> Thanks
> Victoria & Joao
>
> On Tue, Apr 3, 2018 at 11:38 AM Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> PFA updated patch, I've renamed it to query_tool_http_error_handler.js
>> & query_tool_http_error_handler_spec.js respectively.
>>
>> --
>> Regards,
>> Murtuza Zabuawala
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>> On Tue, Apr 3, 2018 at 7:43 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> HI
>>>
>>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala <murtuza.zabuawala@
>>> enterprisedb.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> PFA patch to extract the common code from query tool to handle ajax
>>>> errors & connection handling, Also added unit tests around extracted code.
>>>>
>>>
>>> Looks good to me, except, I wonder if we should rename
>>> is_new_transaction_required.js/is_new_transaction_required_spec.js to
>>> something a little more generic; maybe conn_tx_handler_funcs.js? Not sure I
>>> like that though.
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>