---------- 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 >
