COmanage Developers List

[Benn Oshrin <>]

On 10/10/11 9:46 PM, Marie Huynh wrote:
> In following cake conventions, I added the lft and rght columns that
> are required for the built in tree functions.

I guess now I'm going to have to learn Cake trees :P

> I moved some code to the model and replaced some loops with some
> cakeisms.  Please take a look at the diff file that I've attached.

Most of my comments are style oriented...

[1] According to 
https://spaces.internet2.edu/display/COmanage/COmanage+Coding+Style, we 
agreed on

   if(test)
   {
     foo;
   }
   else
   {
     bar;
   }

This is not followed in several locations.

[2] I'd rename some of the functions. For example, you have 
getParentOptions and findChildrenCous. What's the difference between 
find and get? Also, why Parent (singular) and Children (plural)? 
getParentOptions also seems to me like the function to be called to get 
the options set for the parent (which of course isn't what you're 
doing). So I'd suggest

 getParentOptions -> potentialParents or calculatePotentialParents
 findChildrenCous -> childCous (nb: this is a strange plural,
                     [childCou]s not [children]Cou)

"isSubCou" introduces a new term... what's the difference between "Sub" 
and "Child"? Why isn't this isChildCou?

[3] Make sure comments are correctly styled, especially at the top of a 
function definition. eg:

   // this is a sentence

should be

   // This is a sentence.

[4] I like to be consistent with spacing... so one blank like between 
functions and no spaces between functionName($andItsParameters);

There are various other examples of this.

[5] This block has a couple of problems:

 if(array_search($couNode, $childrenList) == false)
  return false;
 else return true;

First, I'd generally avoid doing explicit comparisons against true and 
false (it's wordy). Instead, just simply use if(array_search()) or 
if(!array_search()).

Second, note [1] applies.

But why isn't it just

 return(array_search($couNode, $childrenList);

?

[6] While you could rewrite the final test of isCoMember to

 return($coOfCou == $currentCou)

in a similar fashion, that seems too obscure to me (since it looks like 
a test with no branches). I'd suggest

 return($coOfCou == $currentCou ? true : false)

instead as the most compact yet legible variation (which might be a good 
general metric: use the most compact code that is still reasonably 
comprehensible).

[7] I find the calling of getParentOptions unsatisfying, given that half 
the time the first parameter will be NULL.

 $options = $this->Cou->getParentOptions($this->data['Cou']['id']);
 $options = $this->Cou->getParentOptions(NULL, $this->cur_co['Co']['id']);

Actually, isn't the latter just "get all COUs for this CO"? In that 
case, perhaps this should be two different function calls...

 $this->Cou->potentialParents($couid);
 $this->Cou->findByCoId($coid);  // Cake magic function

I know a lot of this stuff is nitpicky, but I subscribe to the theory 
that paying attention to details applies across the board, not just to 
"important" stuff, and results in a better product.

-Benn-