MIME-Version: 1.0
In-Reply-To: 
 <CA+yw=mM9knpdKBfiP63t1nO=h2PCgWjPaaWnw+eQ1S6AYy7K8A@mail.gmail.com>
References: 
 <CA+yw=mPCZvZ7+L+g6peQjEe9cihHFUgT4hW3X9ASTRg3wdJgpQ@mail.gmail.com>
	<CA+yw=mORG-LGpa948F8RzbiQkZ3eUmr_3mgqS+akRGx-640Z+Q@mail.gmail.com>
	<CACCA4P1y5u3kmWn+cV7qb9JKCHrAKw-x4kWmbiHSdrcxDfT0bw@mail.gmail.com>
	<CA+yw=mOi=xbgHophMuhw+D9sdmv=rLoubqG8RALQ+1Hw0nZ92g@mail.gmail.com>
	<CA+yw=mOD9i_tLu4r5qpGw1Gd9PKY3xRg4gDHKm010eKPHiCMmQ@mail.gmail.com>
	<CANxoLDedHH+i9hpma8Q3SUiFWU+fMUO_eGtLU+ECmF4fOii_og@mail.gmail.com>
	<CA+yw=mMPQzqmfcpdoJmrvBoA+wc_0758TNDjrF1w1RXHxYsGDw@mail.gmail.com>
	<CANxoLDdESMArY0HvraikVDtBwE4sFqtn0FpSNkohU1R3_vQsNA@mail.gmail.com>
	<CA+yw=mPjs2xqN162encZzWsdUNYBQvKUzHvPcTg3DLirDSe9fA@mail.gmail.com>
	<CA+OCxow0Q5mLbTijw-fQ2_RGpkFTJ-K9hKFPFFxY-NSTLxgJJw@mail.gmail.com>
	<CA+yw=mM9knpdKBfiP63t1nO=h2PCgWjPaaWnw+eQ1S6AYy7K8A@mail.gmail.com>
Date: Mon, 15 Feb 2016 10:48:45 +0000
Message-ID: 
 <CA+OCxoxtSr-POJ6LBF8r9cxKXzTCJdWj1iQsp803-y0arrMsHg@mail.gmail.com>
Subject: Re: patch for cast module
From: Dave Page <dpage@pgadmin.org>
To: Sanket Mehta <sanket.mehta@enterprisedb.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary=089e011602a8d8ee85052bccc2fb
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

--089e011602a8d8ee85052bccc2fb
Content-Type: text/plain; charset=UTF-8

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com
> wrote:

> Hi Dave,
>
> Regarding your suggestion of putting some comments in javascript, I think
> I have already put some comments regarding model data and their controls if
> any extended.
>
> Can you please let me know where exactly you think more comments are
> required?
>

Hi

The issue for me is that jQuery code isn't the easiest to read at the best
of times, with nested/anonymous functions and inline JSON etc. As I look
through the code for the various nodes in isolation, it's extremely
difficult to get a sense of what exactly each part of the code is doing. In
this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)
- Extend the collection class
- Extend the node class
  - Define an init function inline
  - Add the menu options

That part is fairly easy to figure out (easier because there are blank
lines between the logical sections). From there though, it becomes much
harder;

- There are no blank lines to separate logical code sections at all between
line 48 and 235 (there is one blank line, but it doesn't separate code
sections).
- There are 4 comments that I can see. The first two are identical, and
appear to have identical code blocks following them for reasons that are
not even remotely obvious.
- As a newcomer to this code, I'm wondering if it's purpose is to define
the backform model. If so, why is it not broken up into sections with a
comment to tell me what field each block handles, and any other useful
information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the
point is that without spending a couple of hours understanding this, I
simply don't know. The point of the comments (and separation of logical
sections of code with blank lines) is to make it easy for another developer
(especially one as rusty as me) to read and understand, then fix and
improve. Be generous with comments, but don't use them unnecessarily (e.g.
"a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of
us working on pgAdmin need to keep in mind.

Thanks.

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

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

--089e011602a8d8ee85052bccc2fb
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><br><div class=3D"gmail_extra"><br><div class=3D"gmail_quo=
te">On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <span dir=3D"ltr">&lt;<a =
href=3D"mailto:sanket.mehta@enterprisedb.com" target=3D"_blank">sanket.meht=
a@enterprisedb.com</a>&gt;</span> wrote:<br><blockquote class=3D"gmail_quot=
e" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">=
<div dir=3D"ltr"><div><div>Hi Dave,<br><br></div>Regarding your suggestion =
of putting some comments in javascript, I think I have already put some com=
ments regarding model data and their controls if any extended.<br><br></div=
>Can you please let me know where exactly you think more comments are requi=
red?</div></blockquote><div><br></div><div>Hi</div><div><br></div><div>The =
issue for me is that jQuery code isn&#39;t the easiest to read at the best =
of times, with nested/anonymous functions and inline JSON etc. As I look th=
rough the code for the various nodes in isolation, it&#39;s extremely diffi=
cult to get a sense of what exactly each part of the code is doing. In this=
 example, what I see by reading the code is:</div><div><br></div><div>- Def=
ine the required libraries (require.js stuff)</div><div>- Extend the collec=
tion class</div><div>- Extend the node class</div><div>=C2=A0 - Define an i=
nit function inline</div><div>=C2=A0 - Add the menu options</div><div><br><=
/div><div>That part is fairly easy to figure out (easier because there are =
blank lines between the logical sections). From there though, it becomes mu=
ch harder;</div><div><br></div><div>- There are no blank lines to separate =
logical code sections at all between line 48 and 235 (there is one blank li=
ne, but it doesn&#39;t separate code sections).=C2=A0</div><div>- There are=
 4 comments that I can see. The first two are identical, and appear to have=
 identical code blocks following them for reasons that are not even remotel=
y obvious.=C2=A0</div><div>- As a newcomer to this code, I&#39;m wondering =
if it&#39;s purpose is to define the backform model. If so, why is it not b=
roken up into sections with a comment to tell me what field each block hand=
les, and any other useful information I may need to know? If it&#39;s not, =
then what is it for?</div><div><br></div><div>So... I&#39;m not going to te=
ll you exactly where to put comments, because the point is that without spe=
nding a couple of hours understanding this, I simply don&#39;t know. The po=
int of the comments (and separation of logical sections of code with blank =
lines) is to make it easy for another developer (especially one as rusty as=
 me) to read and understand, then fix and improve. Be generous with comment=
s, but don&#39;t use them unnecessarily (e.g. &quot;a =3D 1 // Set a to one=
&quot;).</div><div><br></div><div>Of course, this is not just directed at y=
ou Sanket - it&#39;s something all of us working on pgAdmin need to keep in=
 mind.</div><div><br></div><div>Thanks.</div></div><div><br></div>-- <br><d=
iv class=3D"gmail_signature">Dave Page<br>Blog: <a href=3D"http://pgsnake.b=
logspot.com" target=3D"_blank">http://pgsnake.blogspot.com</a><br>Twitter: =
@pgsnake<br><br>EnterpriseDB UK: <a href=3D"http://www.enterprisedb.com" ta=
rget=3D"_blank">http://www.enterprisedb.com</a><br>The Enterprise PostgreSQ=
L Company<br></div>
</div></div>

--089e011602a8d8ee85052bccc2fb--