You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openmeetings.apache.org by Maxim Solodovnik <so...@gmail.com> on 2012/07/05 08:42:37 UTC
Zimbra plugin code review
Hello Ankur,
I finally was able to find time and review code of your plugin.
1) Why user need 5 different OM servers in his settings?
2) instead of having
var server1_baseurl;
var server1_username;
var server1_password;
....................................
var server5_baseurl;
var server5_username;
var server5_password;
I would recommend you not to store this values but get it by id in runtime
and store values only in the array of objects structure like this:
var credentials = [
{url: "url1", user: "user1", pass: "pass1"}
, {url: "url2", user: "user2", pass: "pass2"}
.........
];
with later access as creadentials[0].url creadentials[0].user etc.
for (var i = 0; i < 5; ++i) {
var baseurl_text = new DwtText({parent:this.pView, name:"server" +
(i+1) + "_baseurl_text", id:"server" + (i+1) + "_baseurl_text"});
baseurl_text.setText("URL");
var baseurl = new DwtInputField ({parent:this.pView, name: "server" +
(i+1) + "_baseurl", id: "server" + (i+1) + "_baseurl"});
.... etc ....
}
{baseurl: "url1", username: "name1", password: "pass1"}
{baseurl: "url2", username: "name2", password: "pass2"}
];
3) The code contains lots of commented code making js file hard to read.
4) calendar_popup_startdate and calendar_popup_enddate as well
as startdate_calendar_okbtnlistener and enddate_calendar_okbtnlistener are
almost identical I would create helper function with general code and use
it.
5) I would change com_zimbra_om._url value from "
http://demo.dataved.ru/openmeetings/services/UserService/getSession"
to com_zimbra_om._url = "http://demo.dataved.ru/openmeetings/services/";
6) in several places URL constructed by concatenating
http://demo.openmeetings.de/openmeetings/services/UserService string with
params. I would create helper function to construct URL:
function getURL(service, function, o) {
var result = com_zimbra_om._url + "/" + service + "/" + function + "?";
if (sid) {
result += "SID=" + sid
}
if (o) {
for (var key : in o) {
result += "&" + key + "=" + escape(o[key]);
}
}
}
and call it like this: var url = getURL("UserService", "getSession");
var url = getURL("UserService", "getRooms", {name: room_name, roomtypes_id:
roomtypes_id, numberOfPartizipants: numberOfPartizipants, ispublic:
ispublic});
etc.
I guess that's it for now :) hope it was helpful.
--
WBR
Maxim aka solomax
Zimbra plugin code review
Posted by Ankur Ankan <an...@gmail.com>.
Okay I will use array.
On Thursday, July 5, 2012, Ankur Ankan <an...@gmail.com> wrote:
> ---------- Forwarded message ----------
> From: "Ankur Ankan" <an...@gmail.com>
> Date: Jul 5, 2012 12:31 PM
> Subject: Re: Zimbra plugin code review
> To: "Maxim Solodovnik" <so...@gmail.com>
>
> Thanks Maxim for your comments :-) You will be seeing a much improved
code the next time.
>
> 1) I thought that the user might have more than one server and I couldn't
find a way in which I could implement the no of servers according to user's
choice.
>
> 2) I was about to change it. I had earlier thought of using an object by
the name of server having three elements baseurl, username and password.
Which would be better an array or object??
>
> 3) I will remove it all the unecessary comments when I commit code to
jira next time.
>
> 4) I will do that.
>
> 5) I think I have left that there by mistake and I guess it is not being
used anywhere.
>
> 6) I will do that also.
>
> On Jul 5, 2012 12:12 PM, "Maxim Solodovnik" <so...@gmail.com> wrote:
>>
>> Hello Ankur,
>> I finally was able to find time and review code of your plugin.
>> 1) Why user need 5 different OM servers in his settings?
>> 2) instead of having
>> var server1_baseurl;
>> var server1_username;
>> var server1_password;
>> ....................................
>> var server5_baseurl;
>> var server5_username;
>> var server5_password;
>> I would recommend you not to store this values but get it by id in
runtime and store values only in the array of objects structure like this:
>> var credentials = [
>> {url: "url1", user: "user1", pass: "pass1"}
>> , {url: "url2", user: "user2", pass: "pass2"}
>> .........
>> ];
>> with later access as creadentials[0].url creadentials[0].user etc.
>> for (var i = 0; i < 5; ++i) {
>> var baseurl_text = new DwtText({parent:this.pView, name:"server" +
(i+1) + "_baseurl_text", id:"server" + (i+1) + "_baseurl_text"});
>> baseurl_text.setText("URL");
>> var baseurl = new DwtInputField ({parent:this.pView, name: "server"
+ (i+1) + "_baseurl", id: "server" + (i+1) + "_baseurl"});
>>
>> .... etc ....
>> }
>> {baseurl: "url1", username: "name1", password: "pass1"}
>> {baseurl: "url2", username: "name2", password: "pass2"}
>> ];
>> 3) The code contains lots of commented code making js file hard to read.
>> 4) calendar_popup_startdate and calendar_popup_enddate as well
as startdate_calendar_okbtnlistener and enddate_calendar_okbtnlistener are
almost identical I would create helper function with general code and use
it.
>> 5) I would change com_zimbra_om._url value from "
http://demo.dataved.ru/openmeetings/services/UserService/getSession"
to com_zimbra_om._url = "http://demo.dataved.ru/openmeetings/services/";
>> 6) in several places URL constructed by concatenating
http://demo.openmeetings.de/openmeetings/services/UserService string with
params. I would create helper function to construct URL:
>> function getURL(service, function, o) {
>> var result = com_zimbra_om._url + "/" + service + "/" + function +
"?";
>> if (sid) {
>> result += "SID=" + sid
>> }
>> if (o) {
>> for (var key : in o) {
>> result += "&" + key + "=" + escape(o[key]);
>> }
>> }
>> }
>> and call it like this: var url = getURL("UserService", "getSession");
>> var url = getURL("UserService", "getRooms", {name: room_name,
roomtypes_id: roomtypes_id, numberOfPartizipants: numberOfPartizipants,
ispublic: ispublic});
>> etc.
>> I guess that's it for now :) hope it was helpful.
>> --
>> WBR
>> Maxim aka solomax
>
Fwd: Re: Zimbra plugin code review
Posted by Ankur Ankan <an...@gmail.com>.
---------- Forwarded message ----------
From: "Ankur Ankan" <an...@gmail.com>
Date: Jul 5, 2012 12:31 PM
Subject: Re: Zimbra plugin code review
To: "Maxim Solodovnik" <so...@gmail.com>
Thanks Maxim for your comments :-) You will be seeing a much improved code
the next time.
1) I thought that the user might have more than one server and I couldn't
find a way in which I could implement the no of servers according to user's
choice.
2) I was about to change it. I had earlier thought of using an object by
the name of server having three elements baseurl, username and password.
Which would be better an array or object??
3) I will remove it all the unecessary comments when I commit code to jira
next time.
4) I will do that.
5) I think I have left that there by mistake and I guess it is not being
used anywhere.
6) I will do that also.
On Jul 5, 2012 12:12 PM, "Maxim Solodovnik" <so...@gmail.com> wrote:
> Hello Ankur,
>
> I finally was able to find time and review code of your plugin.
>
> 1) Why user need 5 different OM servers in his settings?
>
> 2) instead of having
> var server1_baseurl;
> var server1_username;
> var server1_password;
> ....................................
> var server5_baseurl;
> var server5_username;
> var server5_password;
>
> I would recommend you not to store this values but get it by id in runtime
> and store values only in the array of objects structure like this:
> var credentials = [
> {url: "url1", user: "user1", pass: "pass1"}
> , {url: "url2", user: "user2", pass: "pass2"}
> .........
> ];
> with later access as creadentials[0].url creadentials[0].user etc.
>
> for (var i = 0; i < 5; ++i) {
> var baseurl_text = new DwtText({parent:this.pView, name:"server" +
> (i+1) + "_baseurl_text", id:"server" + (i+1) + "_baseurl_text"});
> baseurl_text.setText("URL");
> var baseurl = new DwtInputField ({parent:this.pView, name: "server" +
> (i+1) + "_baseurl", id: "server" + (i+1) + "_baseurl"});
>
>
> .... etc ....
> }
> {baseurl: "url1", username: "name1", password: "pass1"}
> {baseurl: "url2", username: "name2", password: "pass2"}
> ];
>
> 3) The code contains lots of commented code making js file hard to read.
>
> 4) calendar_popup_startdate and calendar_popup_enddate as well
> as startdate_calendar_okbtnlistener and enddate_calendar_okbtnlistener are
> almost identical I would create helper function with general code and use
> it.
>
> 5) I would change com_zimbra_om._url value from "
> http://demo.dataved.ru/openmeetings/services/UserService/getSession"
> to com_zimbra_om._url = "http://demo.dataved.ru/openmeetings/services/";
>
> 6) in several places URL constructed by concatenating
> http://demo.openmeetings.de/openmeetings/services/UserService string with
> params. I would create helper function to construct URL:
> function getURL(service, function, o) {
> var result = com_zimbra_om._url + "/" + service + "/" + function + "?";
> if (sid) {
> result += "SID=" + sid
> }
> if (o) {
> for (var key : in o) {
> result += "&" + key + "=" + escape(o[key]);
> }
> }
> }
>
> and call it like this: var url = getURL("UserService", "getSession");
> var url = getURL("UserService", "getRooms", {name: room_name,
> roomtypes_id: roomtypes_id, numberOfPartizipants: numberOfPartizipants,
> ispublic: ispublic});
> etc.
>
> I guess that's it for now :) hope it was helpful.
>
> --
> WBR
> Maxim aka solomax
>