GRBK-485-1

[jpgorrono, 2011-02-24 04:20:40]

squeaky clean :) Thanks, Jim

[tamsler, 2011-02-24 05:44:41]

Looks good

[tamsler, 2011-02-25 20:31:53]

Looks good. I vote +1 to merge this into trunk.

[Mike W, 2011-03-07 22:19:15]

Looks fine, one comment about consistency.  

+1 to merge.

http://codereview.appspot.com/4223043/diff/1/client/src/java/org/sakaiproject...
File
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/MultiGradeContentPanel.java
(right):

http://codereview.appspot.com/4223043/diff/1/client/src/java/org/sakaiproject...
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/MultiGradeContentPanel.java:1572:
*/
Does this method have to do things this way?  It seems like it will always do
the below if its false, but if it ever gets to be true it will stop checking the
registry.  Either it should be consistent or the constructor/init method should
set it once.

[jimeng_umich.edu, 2011-03-07 23:31:10]

Michael points out an incosistency in the way the config setting is checked.  
If it's false or not set, the code rechecks the setting whenever the method  
is called. If it' true, we never check again. Basically the method is  
expressing disbelief that anyone could be so stupid as to choose a setting  
other than true. :-)

Maybe the method should be less biased. ... is there any circumstance under  
which a config setting can change?  If not, the initial value could be null,  
in which case we could check the config setting. If it's true, we could set  
the value to true and never check again. If it's not true (false or not set)  
we could set it to false and never check again. Would that work?

Good catch, Mike.

Jim


Sent via DROID on Verizon Wireless

-----Original message-----
From: mikewenk@gmail.com
To: jpgorrono@gmail.com, tamsler@gmail.com
Cc: mwenk@ucdavis.edu, jimeng@umich.edu,  
gradebook2-dev@collab.sakaiproject.org, reply@codereview.appspotmail.com
Sent: Mon, Mar 7, 2011 22:19:47 GMT+00:00
Subject: Re: GRBK-485-1 (issue4223043)

Looks fine, one comment about consistency.

+1 to merge.


http://codereview.appspot.com/4223043/diff/1/client/src/java/org/sakaiproject 
/gradebook/gwt/client/gxt/view/panel/MultiGradeContentPanel.java
File
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/MultiGra 
deContentPanel.java
(right):

http://codereview.appspot.com/4223043/diff/1/client/src/java/org/sakaiproject 
/gradebook/gwt/client/gxt/view/panel/MultiGradeContentPanel.java#newcode1572
client/src/java/org/sakaiproject/gradebook/gwt/client/gxt/view/panel/MultiGra 
deContentPanel.java:1572:
*/
Does this method have to do things this way?  It seems like it will
always do the below if its false, but if it ever gets to be true it will
stop checking the registry.  Either it should be consistent or the
constructor/init method should set it once.

http://codereview.appspot.com/4223043/