Re: [GitHub] renctan sent you a message
Pratik Daga
also let me know how to change author name as my pc is own my friends name
@rectan thanks for such a great review and suggestion
----- Original Message -----
From: GitHub <nor...@github.com>
Date: Wednesday, November 3, 2010 11:42 am
Subject: [GitHub] renctan sent you a message
To: pd...@nyu.edu
> renctan sent you a message.
>
> --------------------
> Subject: Comments on Pull Request
>
> First of all, I would like to thank you for devoting your time for
> this project. You did a great job in submitting the code. Please allow
> me to make some comments on your code:
>
> Please cleanup your code - remove codes that you end up not using. For
> example, in query.js, the ReplSetServers you added does not seem to be
> used. Same thing for Db and Admin for connection.js.
>
> connection.js:
> 1. Do you intend to make serverlist a member variable of
> connection.js? Because the way it is right now, it behaves more like a
> static variable (in Java, C++ terminology). Notice that the serverlist
> variable will be overwritten with a new variable everytime the
> ReplSetServers constructor is called. If you want to make it behave
> like a member variable, you should put them inside a method of the
> object you intend it to belong to, for example:
>
> x.prototype.doThis = function () {
> this.serverlist = null; //this is a member variable of an instance of
> x
> };
>
> Yeah, I know, it's weird if you are used to languages like Java & C++,
> but you'll become used to it. I think most scripting languages use
> that idiom as Python and Ruby also use that idiom to create it's own
> "instance member variables".
>
> Nevertheless, you might not need serverlist variable as it appears
> that you are just passing it to the ServerCluster constructor.
>
> 2. Where is serverlistcopy being used?
>
> db.js:
> 1. Same comment on serverlist. Did you intend to use them as member variables?
> 2. Same comment on dbcopy as behaving like static variable. I think
> you may not need dbcopy if you modify checkMaster to either:
>
> //make it accept a db instance
> var checkMaster = function (db) {
> //replace previous instances of dbcopy with db
> };
>
> and to use it at line 543:
> checkMaster(this);
>
> Alternatively, you can make also checkMaster be a method of db:
> Db.prototype.checkMaster = function () {
> //replace previous instances of dbcopy with this
> };
>
> and to use it at line 543:
> checkMaster();
>
> Which approach to use depends on your intent. I am also pretty much
> new to js, so I am not sure of the conventions used in js, but to me,
> the second approach would expose the method to the clients, so it
> seems to behave like public, while the first approach is not visible,
> so it acts more like a private method.
>
> Great job!
> --------------------
>
> Reply on GitHub: http://github.com/inbox/2533177#reply
