[Freeipa-devel] [PATCH] 414-415 Move spec modifications from facet factories to pre_ops

[Petr Vobornik]

On 05/15/2013 04:02 PM, Ana Krivokapic wrote:
> On 05/15/2013 12:09 PM, Petr Vobornik wrote:
>> [PATCH] 414 Move spec modifications from facet factories to pre_ops
>> -------------------------------------------------------------------
>>
>> Spec modifications in factories makes inheritance and extensibility
>> more difficult.
>>
>> Moving them to pre_ops allows modification of their output by other
>> pre_ops.
>>
>> https://fedorahosted.org/freeipa/ticket/3605
>>
>> [PATCH] 415 Unite and move facet pre_ops to related modules
>> -----------------------------------------------------------
>>
>> Facet pre_ops defined in ./facet module were moved to modules where
>> facet are actually defined. Moved pre_ops were united with the ones
>> defined for the facets in these modules.
>>
>> The move simplifies module dependencies - there is no reason to have
>> general facet module dependent on specialized facet modules.
>>
>> Pre_ops uniting makes the code simpler.
>>
>> https://fedorahosted.org/freeipa/ticket/3605
>>
>>
>
> A minor nitpick - in patch 415:
>
> +        if (indirect_members) {
> +            if (indirect_members.indexOf(spec.other_entity) > -1) {
> +                return true;
> +            }
> +        }
>
> This construct:
> if (A) {
>      if (B) {
>          do_something();
>      }
> }
>
> Can be replaced with:
> if (A && B) {
>      do_something();
> }
>
> The second option is more readable, and avoids nested ifs.

Changed:
-        if (indirect_members) {
-            if (indirect_members.indexOf(spec.other_entity) > -1) {
-                return true;
-            }
-        }
-        return false;
+        var has_indirect = !!(indirect_members && 
indirect_members.indexOf(spec.other_entity) > -1);
+        return has_indirect;


Updated patch attached.

>
>
> I have found no other issues, so ACK from me.
>
> --
> Regards,
>
> Ana Krivokapic

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0414-Move-spec-modifications-from-facet-factories-to-pre_.patch
Type: text/x-patch
Size: 11863 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130516/0c607127/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0415-1-Unite-and-move-facet-pre_ops-to-related-modules.patch
Type: text/x-patch
Size: 12874 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130516/0c607127/attachment-0001.bin>