Okay I will use array.
On Thursday, July 5, 2012, Ankur Ankan <[email protected]> wrote: > ---------- Forwarded message ---------- > From: "Ankur Ankan" <[email protected]> > Date: Jul 5, 2012 12:31 PM > Subject: Re: Zimbra plugin code review > To: "Maxim Solodovnik" <[email protected]> > > 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" <[email protected]> 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 >
