COmanage Developers List

[Marie Huynh <>]

Doh.

On Wed, Oct 12, 2011 at 12:53 PM, Marie Huynh 
<>
 wrote:
> Oops, missed an edit.  Heading to a meeting.  I'll check it again after.
>
> On Wed, Oct 12, 2011 at 12:47 PM, Marie Huynh 
> <>
>  wrote:
>> 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