Compass : East Oriented

[srm]

C'est justement pour le moment privé puisqu'il est à destination de mon entreprise
Je n'ai pas pour le moment l'aval de la direction pour le rendre opensource (j'ai pas encore demandé).

Je dis juste que la conversation ne va pas dans le bon sens, puisque je viens parler d'un design de programmation et la discussion dérive sur ce que le PHP permet comme bêtise, ce qui n'était pas trop le sujet initial. Après c'est peut-être moi qui ai apporté le débat dans ce sens, dans ce cas c'est de ma faute ^^

L'exemple peut-être le plus parlant et le plus pertinent "avant/après" East :

Pas East :

    public function current()

    {



        $objects = array();



        foreach ($this->targets as $alias => $columns) {

            foreach ($columns as $column) {

                if (isset($objects[$alias]) === false) {

                    $entityName = $this->entityManager->getClass($column['table']);

                    if ($entityName === null) {

                        continue;

                    }

                    $objects[$alias] = new $entityName();

                }



                $fields = $this->metadataList[$column['table']]->getFields();

                if (isset($fields[$column['name']]['fieldName']) === true) {

                    $propertyName = 'set' . ucfirst($fields[$column['name']]['fieldName']);

                    $objects[$alias]->$propertyName($this->iteratorCurrent[$column['index']]);

                }

            }

        }



        return $objects;

    }

East :

    public function hydrate($columns = array())

    {

        $result       = array();

        $metadataList = array();

        foreach ($columns as $column) {

            if ($column['table'] === '') {

                $column['table'] = 'db__table';

            }



            if (isset($result[$column['table']]) === false) {

                $this->metadataRepository->findMetadataForTable(

                    $column['orgTable'],

                    function ($metadata) use ($column, &$result, &$metadataList) {

                        $metadataList[$column['table']] = $metadata;

                        $result[$column['table']]       = $metadata->createObject();

                    },

                    function () use (&$result, $column) {

                        $result[$column['table']] = new \stdClass();

                    }

                );

            }



            if (isset($metadataList[$column['table']]) === true

                && $metadataList[$column['table']]->hasColumn($column['orgName'])

            ) {

                $metadataList[$column['table']]->setObjectProperty(

                    $result[$column['table']],

                    $column['orgName'],

                    $column['value']

                );

            } else {

                $property = 'db__' . $column['name'];

                $result[$column['table']]->$property = $column['value'];

            }

        }



        return $result;

    }

Mais ça reste un peu foireux car le code East gère plus de cas et est mieux découpé (par exemple dans le cas non East, $this->targets est initialisé en dehors de l'hydrate à proprement parlé)

(Et en effet j'ai réécrit 100% le code en East, j'ai terminé hier et en plus il est 100% testé)

[Xenos]

$a1.foo($a2) { $a2->v } me semble aussi sale, de même que le "private sur le scope que tu veux": un élément privé partagé par différentes classes... Ca sent le gaz et l'usine qui va avec :/

Pas d'accord Oxman: dans le cas de l'attribut publique, on peut écraser la valeur de cet attribut, et y mettre n'importe quoi. Dans le cas du getter seul, on ne peut pas écraser l'attribut. Si on ajoute le setter, celui-ci (via type hinting) peut n'autoriser que certaines classes.
Après, faire un getter qui renvoie l'attribut protégé et un setter sans condition n'est pas différent de faire un attribut publique. La faute, à mon sens, n'est pas de vouloir fuir les attributs publiques, mais de faire des getters et des setters trop laxistes, pas assez restrictifs et qui donc, n'apportent rien.

Oui @niahoo, le code du getter en lui-même ok, ce qui me gène, c'est la formulation qu'on trouverait alors dans la doc, type "getMachinChose(): getter de l'attribut machinChose", qui est la formulation sous-entendue par ton "Le getter te donne du contrôle parce qu'il permet d'avoir un membre privé mais tout de même lisible" (enfin, c'est ce que j'en ai compris). Bref, okay pour le getter en lui-même pas de soucis, mais pas okay pour dire, dans la doc "cette méthode renvoie la valeur de $this.chose", ou "Human::getBrain(): renvoie l'attribut Brain du Human".

J'aime bien mettre des casts inutiles en PHP pour voir clairement ce qu'est retourné par une méthode/fonction: cela évite de fouiller la doc, et en plus, cela assure que l'élément retourné sera bien de ce type. C'est redondant quand on fait un return (int)$objet-toInt(), mais au moins, on est certain que c'est un int qui sera retourné (pratique aussi si on utilise un analyseur de code).

Ah? Le fait qu'il y ai plus de classes, "c'est chiant"? Donc, tu as mis toutes les données de ton PC dans le même fichier (qui doit faire des To de long!) parce que plusieurs fichiers c'est chiant aussi? iffle: Plus de classes, c'est chiant pour avoir une vision d'ensemble et approcher la totalité du projet, mais si c'est bien fait, c'est pas plus mal car cela permet de réduire le code à appréhender.
Si je veux ajouter une fonctionnalité à un projet de 1000Ko en 100 classes, il faudra me farcir au moins une classe, soit 10Ko de code (probablement plus), si j'ai 1000 classes, je réduis potentiellement le code à lire, comprendre puis commiter/merger à seulement 1Ko.

(Oui, PHP permet de faire tout et surtout n'importe quoi ^^)

J'aime pas cet exemple, Oxman, car $metadata dans la function() n'est pas décrit, et parce qu'il devient impossible de changer cette fonction (pas top extensible). Utiliser le DP Command et passer, en paramètre, un objet qui contient la méthode "run($metadata)" me semble plus approprié. Surtout, j'exècre le "use" qui sent l'effet collatéral à plein nez.

// Génère un Command($className) si $className n'est pas null,

// génère une NullCommand sinon

class CommandFactory

{

	public function build($className)

	{

		return ($className === null? new NullCommand(): new Command($className));

	}

}

class Command

{

					$objects[$alias] = new $entityName();

	protected $className;

	public function __construct($className)

	{

		$this->setClassName($className);

	}

	// Juste pour coller un setter ici 

	protected function setClassName($className)

	{

		// Si besoin, ajouter des vérifications sur $className

		$this6>className = $className;

	}



	// Opérations à exécuter

	public function run($that, $column, &$objects, $alias)

	{

		$fields = $that->metadataList[$column['table']]->getFields();

		if (isset($fields[$column['name']]['fieldName']) === true)

		{

			$propertyName = 'set' . ucfirst($fields[$column['name']]['fieldName']);

			$iterator = $that->iteratorCurrent[$column['index']];

			$objects[$alias]->$propertyName($iterator);

		}

	}

}

// Commande ne faisant rien

class NullCommand

{

	public function run()

	{

	}

}

class ...

{

	public function current()

	{

		$objects = array();

		$commandFactory = new CommandFactory();

		foreach ($this->targets as $alias => $columns)

		{

			foreach ($columns as $column)

			{

				if (isset($objects[$alias]) === false)

				{

					$columnTable = $column['table'];

					$entityName = $this->entityManager->getClass($columnTable);

					$command = $commandFactory->build($entityName);

					$command->run($this, $column, $objects, $alias);

				}

			}

		}

		return $objects;

	}

}

Niveau souplesse, on y gagne. Si on fait sauter la dépendance de current() à CommandFactory, on peut même changer la factory, et utiliser autre chose qu'une NullCommand si jamais la classe renvoyée par getClass() est "null" (on peut alors générer un DefaultCommand). Mais on rajoute des classes, donc on limite la vision d'ensemble... ce qui est plutôt bien si on considère l'approche objet et non procédurale Mais c'est moins attrayant pour un p'tit nouveau qui relit le code...

[srm]

Je suis un noob en East style, donc je fais peut-être mal les choses par moment
Je n'ai pas compris ton exemple pour $metadata ?
Tu veux dire que j'aurais du mettre function (Metadata $metadata) ? Si c'est ça tu as parfaitement raison, j'aurais du le faire, j'avoue que je n'y avait pas pensé

Et pour ton exemple de Command tu peux le faire sur l'exemple East pour que je vois bien ce que ça implique ?

[Xenos]

Non, je pensais plutôt à sortir la fonction de la méthode dans laquelle elle est coincée (ne pas passer par une fonction anonyme donc), et documenter cette fonction, pour ainsi pouvoir changer de fonction facilement, et isoler les morceaux de code qui peuvent l'être.

"Reprendre l'exemple East": donc reprendre le tiens avec Command? Cela donnerai, en gros, cela (en jouant sur les espaces de nom pour que Command1 et Command2, spécifiques à la classe "...", soient isolées du reste du projet pour ne pas le polluer):

class Command1

{

	public function run($metadata, $column, &$result, &$metadataList)

	{

		$metadataList[$column['table']] = $metadata;

		$result[$column['table']]       = $metadata->createObject();

	}

}

class Command2

{

	public function run(&$result, $column)

	{

		$result[$column['table']] = new \stdClass();

	}

}



class ...

{

	public function hydrate($columns = array())

	{

		$result       = array();

		$metadataList = array();

		$factory1 = new FactoryCommand1();

		$factory2 = new FactoryCommand2();

		foreach ($columns as $column)

		{

			if ($column['table'] === '')

				$column['table'] = 'db__table';



			if (isset($result[$column['table']]) === false)

			{

				$command1 = $factory1->build();

				$command2 = $factory2->build();

				$this->metadataRepository->findMetadataForTable($column['orgTable'], $command1, $command2);

			}

			if (isset($metadataList[$column['table']]) === true && $metadataList[$column['table']]->hasColumn($column['orgName']))

			{

				$metadataList[$column['table']]->setObjectProperty(

					$result[$column['table']],

					$column['orgName'],

					$column['value']

				);

			}

			else

			{

				$property = 'db__' . $column['name'];

				$result[$column['table']]->$property = $column['value'];

			}

		}

		return $result;

	}

}

Et findMetadataForTable() est en charge d'appeler "->run()" sur l'objet commande qu'on lui a passé au lieu d'appeler la callback. Pour les paramètresà envoyer à run(), ils sont passés par findMetadataForTable() et lors de l'appel à build(), tout dépend de quels paramètres on veut passer.
Ainsi, on a la possibilité de changer le contenu des fonctions exécutées (les méthodes run()). On peut aussi changer la factory, histoire de générer d'autres objets de commandes.

Okay, cela complexifie la lecture du flux d'exécution mais c'est parfaitement normal: seul le procédural permet de suivre bien proprement un flux d'exécution, l'objet est bien plus émergent et laisse donc une place large à la combinatoire qui est fâchée avec les flux d'exécutions prévisibles et débuggables On peut choisir de faire du "procédural caché dans des classes", pour essayer d'avoir les avantages de l'objet (indépendance des classes) et ceux du procédural (flux d'exécution simple à suivre).

Une programmation objet "parfaite" laisse la place large à l'émergence (comme la réalité: on peut utiliser une tringle à rideau comme tuteur de jardin). Paradoxalement, un programme "bien codé objet" est un programme qui bug, puisqu'il laisse émerger des comportements qui, pour certains, peuvent être indésirables; l'idéal étant alors d'être capable de distinguer les bons et mauvais comportements qui émergeront et de rattraper ces derniers.

---

Mais attention: la classe que tu proposes est parfaitement valide (enfin, si elle marche); c'est seulement s'il vient le moment où il faut étendre ses possibilités que le découpage se justifie. Sur-découper sans en avoir besoin, pour avoir des moissons de classes, ce n'est pas utile. Découper parce qu'on souhaite modifier le comportement d'une classe ou permettre sa modification (si API ou dev en équipe par exemple), c'est justifié.

[srm]

Je suis d'accord là dessus, normalement on pousse le truc pour faire comme tu dis.
Mais quand c'est pour mettre un objet qui ne sert à rien de particulier j'ai pour le moment mis des callback.
C'est pour ça que j'ai dit que j'étais pas "East jusqu'auboutissime", si ça n'apporte rien les objets, j'ai mis des callback et je pourrais remplacer ça par des objets si je viens à avoir le besoin de souplesse.

[niahoo]

$a1.foo($a2) { $a2->v } me semble aussi sale, de même que le "private sur le scope que tu veux": un élément privé partagé par différentes classes... Ca sent le gaz et l'usine qui va avec :/[quote]

Ah non mail il y a un malentendu. Cette syntaxe est bien sur encore pire qu'un getter. Si tu relis mon code je disais juste que PHP permet ce genre de choses alors que ça va à mon sens à l'encontre de l'OO. Comme le disait oxman si on fait avec East alors on ne touche pas aux variables. D'où la nécessité des getters dans tous les cas.

[quote]Oui @niahoo, le code du getter en lui-même ok, ce qui me gène, c'est la formulation qu'on trouverait alors dans la doc, type "getMachinChose(): getter de l'attribut machinChose", qui est la formulation sous-entendue par ton "Le getter te donne du contrôle parce qu'il permet d'avoir un membre privé mais tout de même lisible" (enfin, c'est ce que j'en ai compris). Bref, okay pour le getter en lui-même pas de soucis, mais pas okay pour dire, dans la doc "cette méthode renvoie la valeur de $this.chose", ou "Human::getBrain(): renvoie l'attribut Brain du Human".

Ok ! Effectivement je me plaçais dans une logique ou on avait à la base un attribut public, et on se dit « hola, pas touche » et *paf* getter. La refactorisation nécessaire après ça est simple donc ça se fait. Mais ensuite oui pour de la doc, la doc doit simplement dire que le getter renvoie telle info, sans dire comment elle est stockée. Note que le code appelant peut savoir que la donnée est tout de même stockée en base de données ; pas forcément.

Ce qui est chiant c'est d'avoir plus de classes que nécessaire. Dans l'article on voit clairement que pour avoir les films par auteur ça prend trois fonctions mais le mec t'envoie une tartine de classes. Alors OK, éventuellement peut-être un jour si dieu le veut et si les planètes sont bien alignées on voudra que l'appli renvoie aussi les films classées par ordre alphabétique inverse du dernier acteur blond de plus de 24 ans et demi à apparaître dans le générique. Mais sinon le finder ça va quoi. Soit ton ORM l'a soit tu t'en passes.

Fes fichiers font la taille qu'ils doivent. Mais j'ai eu par exemple des albums de musique en un seul mp3 avec le fichier d'index qui va avec.

Bon en fait je viens de lire tes exemples de code et quand je vois le code d'oxman et ce qu'il est censé faire (et j'ai pas compris en quoi le 2eme code était plus East), je pense qu'on ne se comprendra pas car tu te shootes beaucoup plus que moi à l'abstraction.

Par contre tu as fait une erreur. Tu as oublié d'implémenter une CommandFactoryFactory qui définit une CommandFactory en fonction du type d'objets sur lesquels la commande va agir. Mais au lieu de t'emmerder à le faire, tu peux simplement utiliser une CommandFactoryFactoryFactory, c'est le plus simple.

(je blague, ça va c'est lisible pour moi).

---

Et sinon petite digression : un ORM en 600 lignes de PHP ça me ferait rêver. Vous allez casser tous mes rêves avec vos factories. Bon, ma question : Oxman, pourquoi est-ce que tu fais un ORM (question sérieuse, c'est pas du troll). Est-ce ta boite qui veut absolument du code maison, est-ce toi qui les a poussés à choisir une solution maison parce qu'en implémenter un t'intéresse, ou bien ...

[srm]

Mon premier code à 2 getters, "tell don't ask"
Mon deuxième code à 0 getters

C'est pas vraiment un ORM, mais plutôt un OM (objet mapper) et il est pas fini, il fera sans doute 1200 lignes à la fin, ou un peu moins. J'aime bien coder des "ORM". On allait utiliser Doctrine et il est trop gourmand en mémoire, sans compter que par moment tu peux avoir des problématiques bien compliquer à résoudre avec Doctrine. Et en dehors de Doctrine il n'y a pas grand chose comme ORM robuste, fiable et suivis.

Et surtout, on a des besoins de perf accrus et des besoins un peut spéciaux du genre, avoir 5 ou 6 type de connections (mysql et postgresql) par site il faut que ça soit transparent dans les modèles etc.

[niahoo]

Oui ok pour les getters. Bon tu "ask" quand même createObject et hasColumn mais c'est pour chipoter ! Après ça me semble sympa.

Ok sinon thx

[Xenos]

(Oui, la factory dans la méthode elle-même n'a aucun intérêt, elle devrait venir de l'extérieur soit d'un attribut de la classe, soit d'un paramètre de méthode, voire même d'un objet "global" type singleton").
Clairement, j'aurai gardé le premier code, c'est seulement sa refactorisation East qui me gènait.

J'aime bien voir le procédural comme une chaîne métalique, et l'OO comme un vrac de maillons dans un panier
La chaîne métallique, c'est intuitif, c'est utilisable directement, mais en revanche, cela ne permet pas d'accrocher 3 ampoules à une suspension du plafond. En revanche, le vrac de maillons dans un panier, c'est très souple, on peut en faire plein de choses (refaire une chaine simple ou trois petits chaines ou une boucle) et suspendre ce qu'on veut au plafond. Mais casser la chaine du procédural pour le plaisir d'avoir un vrac de maillons que l'on remonte ensuite pour refaire la même chaîne après s'être bien pris la tête, cela ne sert à rien...
Du coup, rêver du nombre de lignes de code... mouais... Cela dépend vraiment de la découpe (1 classe de 1200 lignes? 1000 classes de 2 lignes? pour faire les deux extrêmes!) et de la souplesse qu'on en attend.
Mais clairement, s'il y a besoin de performances accrues, faut rester sur la première solution; je n'ai plus les chiffres sous la main, mais les appels de méthodes en PHP sont de véritables enclumes et forment vite la majorité du temps d'exécution si on s'amuse à hyper abstraire son code (ou à passer par des callbacks ).

[srm]

createObject c'est une factory donc ça n'est pas un getter en tant que tel.
Et hasColumn ne demande aucune donnée, je lui dis juste de me dire si il a la colonne ou pas.
Donc ça ne sont pas des getters à mon sens

On a besoin de performances accrues mais toute proportion gardée, par exemple on a pas envisagé à un seul moment de tout coder en procédural pour que ça soit plus rapide
Et de coder en East n'est pas plus lent que de coder en Objet procédurale.