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

Thanks, applied.

On Wed, Apr 4, 2018 at 9:43 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> 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(dot)zabuawala(at)enterprisedb(dot)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(dot)zabuawala(at)enterprisedb(dot)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
>>>>
>>>
>>>
>

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

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