Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f3R6v-0000ei-HI for pgadmin-hackers@arkaria.postgresql.org; Tue, 03 Apr 2018 18:56:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f3R6t-0008RD-MY for pgadmin-hackers@arkaria.postgresql.org; Tue, 03 Apr 2018 18:56:47 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f3R6s-00085E-8O for pgadmin-hackers@lists.postgresql.org; Tue, 03 Apr 2018 18:56:46 +0000 Received: from mail-io0-x22a.google.com ([2607:f8b0:4001:c06::22a]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f3R6n-0007BX-Fe for pgadmin-hackers@postgresql.org; Tue, 03 Apr 2018 18:56:44 +0000 Received: by mail-io0-x22a.google.com with SMTP id v13so23266988iob.6 for ; Tue, 03 Apr 2018 11:56:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rthBJWBHb2WqCuU/AkKRiUYj15auVogB65c1JjbROsw=; b=cbkHApUeHWxb28yaBj19G/Sy+vLsTLj7KfVnzQEzMzP/deytiNH7CaswR8t+01E0WN CpmMDHatM5hms4qS7LIEoEhaOCJuOZ0fFCgYBvEKWxulJdsk2ilJQj3eWfezmdn/5qDb zB6v1w8zNdLgUk7B7nqigkuzIY6CinTQD53STIav9/RNhLogcRqCAmXVK4KBAscuQCvo cjIW3tCNBUUt9LOGSZfQ0NdPiQCCCEzar7Ns1yuVKiEyfqb/lRtUtwM4BEv0qCyIMG3/ QjujEqJszzsvVKpdYM1aFc4Db6kFlm0HAJbCEcWf93K2Y+mREbM4wfbvD1c2AIzW3UxQ 95Mg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rthBJWBHb2WqCuU/AkKRiUYj15auVogB65c1JjbROsw=; b=q8uHJ4p6j3mLVbUSzvcqbK0f25Xmbxiko7lWxvUo40XgfyROpgmW4386qlEDSCDGw8 OnH5kNCD4z9qM7K6E7TQeldxIca/k0booLtatX2t92r+He64pQ88e79xI32WbwelaS6x IxINx0q8xnRR9fD/l9lMi8/8AT/tSbENv3E/ZV3grh8a82Y1dtsS1YbsaS+6qNTVUNBz K5UmW7Cag24hsvfk+HjOD5bwWsm6PqPmG/7fxsfRdDT1o5DOuUbUGFbbKxpHOdDzpu4D SObw2bHH0AQL7e3A5osPPlkQ0t37T7ZVLLVywC7NWr7zhOOpaan/S+9G/rhF7BPyVwIT MjSw== X-Gm-Message-State: AElRT7Ews9Kh49QYiyJXxuzkLAMuuA/9yntrpfbkC8BSljy8gL5IadSG VSUvBfMp0OL/XsmIu70e5MH+wWm09ZnlT7UrTB/5xA== X-Google-Smtp-Source: AIpwx4/5ULdnuaeL9uLXTF/AcA4dVJ/1lOzFFIvwVpIt6V9bLy8KfdqdGClwRkOqiAeioDvTp3jLE/+MefWU3BiijNU= X-Received: by 10.107.55.9 with SMTP id e9mr13796283ioa.69.1522781799975; Tue, 03 Apr 2018 11:56:39 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Tue, 03 Apr 2018 18:56:29 +0000 Message-ID: Subject: Re: [pgAdmin4][RM#3235] Code refactoring in Query tool To: Murtuza Zabuawala Cc: Dave Page , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a114ab7724267fc0568f6440e" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a114ab7724267fc0568f6440e Content-Type: text/plain; charset="UTF-8" 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 < 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 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 >> > > --001a114ab7724267fc0568f6440e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Murtuza

It is really good to see oth= er patches that are just refactoring code.

We have= some suggestions:
- We are trying to use =3D=3D=3D instead of = =3D=3D because =3D=3D=3D ensure that the type is also checked (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equa= lity_comparisons_and_sameness)
- Now that we are refactoring = some code, maybe we should keep some consistency on the way we name functio= ns and variables.
We should use camelCase for names instead of sn= ake_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 bett= er to refactor it.
- By the name of the function=C2=A0is_new_transaction_required, = it describes what the function represents but doesn't seem to explain t= he full scope of the function. What do you think about the name:=C2=A0httpResponseRequiresNewTransac= tion=C2=A0=C2=A0
- The extraction doesn't look like it= matches the code removed

-=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 if (pgAdmin.Browser.UserManagement.is_pga_login_re= quired(e)) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 se= lf.save_state('_explain_timing', []);
-=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return pgAdmin.Browser.UserManagement.pga_l= ogin();
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
-<= /div>
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if(transaction.is_new_= transaction_required(e)) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 self.save_state('_explain_timing', []);
-= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.init_transacti= on();
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 alertify.alert(gettext(&= #39;Explain options error'),
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 gettext('Error occurred while setting timing option i= n explain.')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 let m= sg =3D httpErrorHandler.handleQueryToolAjaxError(
+=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pgAdmin, self, e, '_explain_timing&#= 39;, null, true
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0)= ;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 alertify.alert(gette= xt('Explain options error'), msg);
In this case we = are only saving state if the following conditions are true:=C2=A0
isNotConnectedToThePythonServer and httpResponseJSONIsPresent and connecti= onLostToPostgresDatabase and shouldSaveState
That is not the case= on the removed code.
- The functions extracted when are called d= o not use all the parameters. This tells us that the function groups too mu= ch 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 Murtu= za Zabuawala <murt= uza.zabuawala@enterprisedb.com> wrote:
Hi Dave,
PFA updated patch, I've renamed it to=C2=A0query_= tool_http_error_handler.js &=C2=A0query_tool_http_error_handler_spec.js= respectively.=C2=A0

--
Regards,


On Tue, Apr 3, 2018 at 7:43 PM, Dave Page <= dpage@pgadmin.org> wrote:
<= div dir=3D"ltr">HI

On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala &= lt;= 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 co= de.=C2=A0

Looks good to me, exc= ept, I wonder if we should rename is_new_transaction_required.js/is_new_transaction_required_spec.js to something a little m= ore generic; maybe conn_tx_handler_funcs.js? Not sure I like that though.


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

Enterp= riseDB UK: http:/= /www.enterprisedb.com
The Enterprise PostgreSQL Company

--001a114ab7724267fc0568f6440e--