COmanage Developers List

[Marie Huynh <>]

Please see attached diff.

On Tue, Oct 11, 2011 at 1:49 PM, Benn Oshrin 
<>
 wrote:
> 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-
>

Attachment: co172diff
Description: Binary data