Bug in Contenido_FrontendNavigation::isActiveChild

[sier]

Hallo

musste etwas an der Top-Navigation herumbasteln. Dabei habe ich nachfolgenden Bug entdeckt. Wie und wo werden eigentlich Bug gemeldet??

Code: Alles auswählen

	public function isActiveChild(Contenido_Category $oCategory, $iCurrentIdcat) {
		if ($oCategory->getSubCategories()->count() > 0) {
			$iCurrentIdcat = (int) $iCurrentIdcat;
			$oChildCategories = $oCategory->getSubCategories();
			foreach ($oChildCategories as $oChildCat) {
				if ($oChildCat->getIdCat() == $iCurrentIdcat) {
					return true;
				}
				$this->isActiveChild($oChildCat, $iCurrentIdcat)
			}
			return false;
		}
		return false;
	}

Richtig wäre aber :

Code: Alles auswählen

	public function isActiveChild(Contenido_Category $oCategory, $iCurrentIdcat) {
		if ($oCategory->getSubCategories()->count() > 0) {
			$iCurrentIdcat = (int) $iCurrentIdcat;
			$oChildCategories = $oCategory->getSubCategories();
			foreach ($oChildCategories as $oChildCat) {
				if ($oChildCat->getIdCat() == $iCurrentIdcat) {
					return true;
				}
				if ($this->isActiveChild($oChildCat, $iCurrentIdcat)) {
					return true;
				}
			}
			return false;
		}
		return false;
	}

[idea-tec]

Wo Bugs drauf steht ist auch Bugs drin:
http://forum.contenido.org/viewforum.php?f=63

[_wiewo_]

Ähm bin ich gerade nur blöd oder blind?
Der originalcode is doch 100% richtig oder? oO
is doch alles genau wie es soll

wozu nen if nochmal rumpacken wenns doch eh nochmal neu durchläuft,
nene, kein bug =)

[Oldperl]

Kein Bug
Die Funktion wird rekursiv aufgerufen bis die Bedingung passt oder keine weiteren Elemente zu prüfen sind. Die Rückgaben sollten dementsprechend korrekt sein.

Gruß aus Franken

Ortwin

[sier]

welche rückgabe? bei rekursiven aufruf wird die rückgabe ja gar nie abgefragt/berücksichtigt.

Code: Alles auswählen

         foreach ($oChildCategories as $oChildCat) {
            if ($oChildCat->getIdCat() == $iCurrentIdcat) {
               return true;
            }
            $this->isActiveChild($oChildCat, $iCurrentIdcat) --> WENN hier TRUE zurückkommt muss der foreach abgebrochen werden
         }

[_wiewo_]

Code: Alles auswählen

foreach ($oChildCategories as $oChildCat){
if ($oChildCat->getIdCat() == $iCurrentIdcat) {
  return true;
}
$this->isActiveChild($oChildCat, $iCurrentIdcat);
}

if, also "wenns" so ist, dann returnd er true und fertig is die kiste
nachem return is aus die maus
wenn nich true dann rufter die funktion nochmal auf
isses wieder nich true rufterse nochmal auf
usw,
daher ist das alles schon so richtig wie es da ist

man könnt auch schreiben

Code: Alles auswählen

foreach ($oChildCategories as $oChildCat) {
if ($oChildCat->getIdCat() == $iCurrentIdcat) {
  return true;
} else
{
  $this->isActiveChild($oChildCat, $iCurrentIdcat);
}
}

was aber aufs gleiche hinausläuft

[sier]

nein der code funktioniert nicht.
glaubs mir, mein code macht völlig etwas anders.
in deinem code wird die suche nicht abgebrochen wenn das durchsuchen der unterkategorien erfolgreich war.
dein code funktioniert nur auf dem einem level

gruss
rocco

[idea-tec]

wäre es möglich, dass hier verschiedene Gedankengänge über die Arbeitsweise der Funktion existieren?

[Oldperl]

nach ein bisserl Nachdenken im Kämmerchen glaube ich das rocco Recht hat. Es fehlt zwar keine if-Bedingung, bzw. diese ist hier nicht nötig, denn es reicht einfach das Ergebnis des rekursiven Aufrufes zu returnen, da hierbei auf alle Fälle entweder False oder True zurückkommt.

Code: Alles auswählen

foreach ($oChildCategories as $oChildCat){
if ($oChildCat->getIdCat() == $iCurrentIdcat) {
  return true;
}
return $this->isActiveChild($oChildCat, $iCurrentIdcat);
}

Bevor ich das aber nach Bugs verschiebe werde ich mir das in einer 4.8.12er mal genauer anschauen.

Gruß aus Franken

Ortwin

[Oldperl]

wäre es möglich, dass hier verschiedene Gedankengänge über die Arbeitsweise der Funktion existieren?

@Karsten
ich denke nicht, bei mir wars einfach beim Überfliegen mal wieder zu schnell gewesen

Gruß aus Franken

Ortwin

[sier]

wäre es möglich, dass hier verschiedene Gedankengänge über die Arbeitsweise der Funktion existieren?

schon möglich, hier die funktionsbeschreibung: "Check if current idcat is an active child category of a given Contenido_Category"

aber der aufruf

Code: Alles auswählen

$this->isActiveChild($oChildCat, $iCurrentIdcat)

innerhalb der foreach-scheife im orignal code ist ein bug, unabhängig wie der arbeitsweise gemeint ist.

gruss
rocco

[idea-tec]

wenn es am ende so etwas wie ein bug war/ist, und er wird beseitigt, und wir reduzieren dadurch das feuer auf die db -> SUPER!!!

@Oldperl:
ich hatte mir das "nachsehen" auch notiert (aus reiner neugier), stecke aber zu tief in den letzten Zügen meines projektes und komme vor mitte/ende nächster woche sicher nicht dazu.
von daher, streiche ich mir das wieder, da ich das "genauer anschauen" nun in guten Augen weiß.

[_wiewo_]

von der PHP Seite... (http://php.net/manual/de/functions.user-defined.php)

Beispiel #4 Rekursive Funktionen

Code: Alles auswählen

<?php
function recursion($a)
{
    if ($a < 20) {
        echo "$a\n";
        recursion($a + 1);
    }
}
?>

oder

http://pear.php.net/manual/de/standars.errors.php

Code: Alles auswählen

<?php
/**
 * Rekursive Suche in einem Baum nach einem String
 * @throws ResultException
 */
public function search(TreeNode $node, $data)  
{
    if ($node->data === $data) {
         throw new ResultException( $node );
    } else {
         search( $node->leftChild, $data );
         search( $node->rightChild, $data );
    }
}
?> 

Also die Funktion einfach so wieder aufrufen ist vom PHP Sinn der Rekursion
Das sind zwar keine Beispiele mit return, aber...
Ein return mittendrin einzubauen wäre meines Erachtens kein sauberer CodingStyle.
Ich würde niemals direkt vor einen Funktionsaufruf ein Return setzen, das gehört sich nich. meines Erachtes, Für andere Meinungen bin ich offen
Finde das ein Interessantes Thema

[idea-tec]

ich bin ja nun selfmade-php und denke, dass das mit dem return schon geht.
ich würde dann allerdings auf jeden fall direkt hinter das return ein exit setzen, weil ich ja mein "ergebnis" habe und auf keinen fall will, dass das ding weiter da durchrauscht.
vor allem könnte es ja auch sein, dass ich das ergebnis nach der 4. von 120 schleifen habe und dann rauscht das noch weitere 116 schleifen da durch, ohne sinn und zweck

[_wiewo_]

nach nem return bricht er eigentlich sofort ab

return

Wird die return() Anweisung innerhalb einer Funktion aufgerufen, wird die Ausführung der Funktion sofort beendet und das Argument als Wert des Funktionsaufrufs zurückgegeben.

http://www.php-resource.de/handbuch/function.return.htm