<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 05/15/2013 12:09 PM, Petr Vobornik
wrote:<br>
</div>
<blockquote cite="mid:51935EE4.70706@redhat.com" type="cite">[PATCH]
414 Move spec modifications from facet factories to pre_ops
<br>
-------------------------------------------------------------------
<br>
<br>
Spec modifications in factories makes inheritance and
extensibility more difficult.
<br>
<br>
Moving them to pre_ops allows modification of their output by
other pre_ops.
<br>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/3605">https://fedorahosted.org/freeipa/ticket/3605</a>
<br>
<br>
[PATCH] 415 Unite and move facet pre_ops to related modules
<br>
-----------------------------------------------------------
<br>
<br>
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.
<br>
<br>
The move simplifies module dependencies - there is no reason to
have general facet module dependent on specialized facet modules.
<br>
<br>
Pre_ops uniting makes the code simpler.
<br>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/3605">https://fedorahosted.org/freeipa/ticket/3605</a>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
A minor nitpick - in patch 415:<br>
<pre wrap="">+ if (indirect_members) {
+ if (indirect_members.indexOf(spec.other_entity) > -1) {
+ return true;
+ }
+ }
</pre>
This construct:<br>
if (A) {<br>
if (B) {<br>
do_something();<br>
}<br>
}<br>
<br>
Can be replaced with:<br>
if (A && B) {<br>
do_something();<br>
}<br>
<br>
The second option is more readable, and avoids nested ifs.<br>
<br>
<br>
I have found no other issues, so ACK from me.<br>
<pre class="moz-signature" cols="72">--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.</pre>
</body>
</html>