From: | Sanket Mehta <sanket(dot)mehta(at)enterprisedb(dot)com> |
---|---|
To: | Dave Page <dpage(at)pgadmin(dot)org> |
Cc: | pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org> |
Subject: | Re: patch for cast module |
Date: | 2016-02-15 13:28:58 |
Message-ID: | CA+yw=mPfUbTk_J-ddArjRCruNGZ0_LFAB6CQY77PODPif8jHsA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
Hi,
PFA the revised patch with all the required comments.
Regards,
Sanket Mehta
Sr Software engineer
Enterprisedb
On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>
> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <
> sanket(dot)mehta(at)enterprisedb(dot)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
>
Attachment | Content-Type | Size |
---|---|---|
castv8.patch | text/x-patch | 40.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2016-02-15 16:53:10 | Re: patch for cast module |
Previous Message | Akshay Joshi | 2016-02-15 13:02:04 | [pgAdmin4] Numeric Control for Backform |