public inbox for [email protected]  

help / color / mirror / Atom feed

From: Joao De Almeida Pereira <[email protected]>
To: Murtuza Zabuawala <[email protected]>
Cc: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
Date: Tue, 03 Apr 2018 18:56:29 +0000
Message-ID: <CAE+jjang5oqj8-c_VhryAET8aaj6USYtRuLmY8gKctC8wz475w@mail.gmail.com> (raw)
In-Reply-To: <CAKKotZRwxcozrSic25xSCtK_z7bdnksrF2_4GSeWi=6zDUSw_g@mail.gmail.com>
References: <CAKKotZRidfdPkSe=2wiaDsz_F3sAXdWZs_1SQLFks1YeP24UTQ@mail.gmail.com>
	<CA+OCxoygT-+xezSAboURa9E9myQCX2GbO4DCFg7yY+umHhEa5g@mail.gmail.com>
	<CAKKotZRwxcozrSic25xSCtK_z7bdnksrF2_4GSeWi=6zDUSw_g@mail.gmail.com>

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
)
- 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.
- 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:
httpResponseRequiresNewTransaction
- 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.
- 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.


Thanks
Victoria & Joao

On Tue, Apr 3, 2018 at 11:38 AM Murtuza Zabuawala <
[email protected]> 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 <[email protected]> wrote:
>
>> HI
>>
>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala <
>> [email protected]> 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
>>
>
>

view thread (6+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
  In-Reply-To: <CAE+jjang5oqj8-c_VhryAET8aaj6USYtRuLmY8gKctC8wz475w@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox