Salut les grafikos,
Pardonner moi si cette question à déjà été posé c'est mon pemier post ici. J'ai bien cherche et pas trouvé de réponse. Et finalement c'est plus votre avis qui m'interesse.
J'ai pas d'exemple professionnel pour codé ni de prétention a le devenir, juste moultes tutoriaux ecumé durant ces 11 dernières années, et une passion grandissante pour le webdev.
Actuellement je dévelloppe une application pour mon entreprise.
Et je me retrouve à codé un objet (model) qui aujourd'hui fait 1187 lignes.
C'est ici ma première question : 1187 lignes pour un objet ? Sa vous arrive ou ai-je peut être abusé quelque part ?
Du coup je commence serieusement a me posé des questions sur la clareter du code ayant gouter depuis peut à l'organisation M.V.C.
Je sait pas trop si vous aller comprendre la ou je veut en venir.
Autre question qui fait suite à la première : Je me retrouve avec une method qui parcours un tableau dans un fetchAll puis qui trie les elements dont j'ai besoin par un foreach et les stocks dans des tableaux différents. Et ce repeter 3 fois dans un for (A mes yeux sa commence a faire crade comme code mais j'ai pas trouvé plus optimisé a mon niveau)
Ce qui me fait une method de 110 lignes avec moulte if 1for 2foreach (si le premier n'a pas fonctionner c'est le suivant qui fait sont affaire)
ce qui fait 3 foreach pour le model et 3 pour le view (rien que pour afficher la page)
Aujourd'hui je me dit qu'il serait peut etre plus lisible de faire d'autres method pour diviser ce code et l'aérer, mais dans ce cas ça serait moin optimisé (augmenterai le nombre de for et foreach)
Comment vous organisez vous fasse a ce genre de dilemme ? L'optimisation est plus important fasse a la lisibilité ?
Est-ce que je me prend le chou pour rien ?
Parce que le code parle mieu que ce grand texte, je vous met la method en question
public function __construct(){
$this->DB = PDO2::getInstance(); //Instance PDO
//Lecture de la table pour aujourd'hui
$register = $this->DB->query('SELECT * FROM registre_tmb WHERE d_dem = "'.date('Y-m-d').'" ');
$this->arrayToday = $register->fetchAll(PDO::FETCH_ASSOC);
//Trois boucle pour 3 Lignes
for($i = 1; $i <= 3; $i++ ){
//Creation des variables pour modifications dans boucle
$Ligne = 'Ligne'.$i;
$Time = 'Time'.$i;
//Exploration du tableau du jour
foreach( $this->arrayToday as $key ) {
//On prend les données concernant uniquement la lignes en cours
if( $key['ligne'] == $i ){
//Dernier entré du tableau concernant $i (Pour position commandes)
$this->{$Ligne} = arrayToObject($key);
if( empty($this->{$Time}['t_first'] ) ){
$this->{$Time}['t_first'] = $key['h_dem'];
}
//Calcul le temps d'arret
if( !is_null($key['h_dem']) ){
//Si il y a une arrêt de stocker en temporaire
if( isset($this->{$Time}['previous_arret']) ){
//Utilisation de la fonction de calcul qui fait la difference ( voir plus bas )
@$this->{$Time}['t_arret'] += $this->CalculDiff($this->{$Time}['previous_arret'], $key['h_dem']);
}
}
//Calcul le temps de fonctionnement si l'heure d'arrêt et defini
if( !is_null($key['h_arret']) ){
//Fonctione de calcul qui fait la difference ( un peut plus bas )
@$this->{$Time}['t_marche'] += $this->CalculDiff($key['h_dem'], $key['h_arret']);
//Stockage en temporaire de l'heure d'arrêt
$this->{$Time}['previous_arret'] = $key['h_arret'];
//Stockage du dernier arrêt de la ligne (Calcul temps de fonctionnement total)
$this->{$Time}['t_last'] = $key['h_arret'];
}else{
$this->{$Time}['t_last'] = date('H:i');
}
$this->{$Time}['t_total'] = $this->CalculDiff($this->{$Time}['t_first'], $this->{$Time}['t_last']);
//Converne uniquement la ligne 1 est 2
if( $i <= 2 ){
//Enregistrement de la premiere heure d'alimentation uniquement si vide
if( empty($this->brs['first_brs'.$key['brs']]) ){
$this->brs['first_brs'.$key['brs']] = $key['h_dem'];
}
//Enregistrement de la derniere heure d'arrêt sur ce brs
//Si NULL mise en temps reèl
$this->brs['last_brs'.$key['brs']] = is_null($key['h_arret']) ? date('H:i') : $key['h_arret'] ;
$this->brs['alim_brs_'.$key['brs']] = $this->CalculDiff($this->brs['first_brs'.$key['brs']], $this->brs['last_brs'.$key['brs']]);
}
}
$this->{$Time}['newDay'] = false;
}
//Si ont a pas plus remplir avec les données du jours
if( empty($this->{$Ligne}) ){
$this->RegistreYesterday();
//Parcours du tableau de la veille pour les commandes
foreach( $this->arrayYesterday as $key ) {
//On prend les données concernant uniquement la lignes en cours
if( $key['ligne'] == $i ){
//Dernier entré du tableau concernant $i (Pour position commandes)
$this->{$Ligne} = arrayToObject($key);
//Passage de la variable "nouveau jour" a vrai
$this->{$Time}['newDay'] = true;
}
}
}
$this->{$Time} = arrayToObject($this->{$Time});
}
$this->ReturnCycle();
$this->ReturnCommandes();
$this->MidnightHour();
}Si vous avez tenu jusqu'ici Chapeau... Et merci pour votre interet.
4 réponses
Moi j'aurais tendance à créer une classe pour la manipulation des tableaux : http://www.grafikart.fr/tutoriels/php/poo-collection-php-523
Ce qui du coup te permettrait d'alléger ton code au niveau du constructeur
Arhem... 1187 lignes pour une class, ce n'est pas complètement abusé, mais le constructeur m'a l'air un peu gros ^^
Il est vrai que le constructeur etait pas mal.
Une petite cure lui à fait un bon coup de vide surtout en utilisant la class pour manipuler les tableaux (Merci le Raton Laveur ;) qui ma obliger a revoir le code.
Refactering est le bon mot pour désigner ce que tu dois faire. Met toi à ma place qui dit ton code, toutes les conditionnelles que je dois comprendre. Ton constructeur pourrait être assurément mieux. Prenons quelques minutes pour traiter une conditionnel...
//Calcul le temps d'arret
if( !is_null($key['h_dem']) )
{
//Si il y a une arrêt de stocker en temporaire
if( isset($this->{$Time}['previous_arret']) )
{
//Utilisation de la fonction de calcul qui fait la difference ( voir plus bas )
@$this->{$Time}['t_arret'] += $this->CalculDiff($this->{$Time}['previous_arret'], $key['h_dem']);
}
}Bon, première chose quand je vois ce que. Je lis les commentaires pour comprendre ce que ça fait. Le problème c'est que je te fait confiance sur ce que tu me dis ce que la conditionnelle vérifie exactement, le code ne parle pas.
<?php
private function tempsArrêtEstCalculé($key)
{
return !is_null($key['h_dem']);
}
// [...]
if($this->tempsArrêtEstCalculé())
{
//Si il y a une arrêt de stocker en temporaire
if( isset($this->{$Time}['previous_arret']) )
{
//Utilisation de la fonction de calcul qui fait la difference ( voir plus bas )
@$this->{$Time}['t_arret'] += $this->CalculDiff($this->{$Time}['previous_arret'], $key['h_dem']);
}
}Je viens de modularisé une seule conditionnel, nous permettant de retirer le commentaire, pour une méthode sans ambiguité.
Continuons...
<?php
private function tempsArrêtEstCalculé($key)
{
return !is_null($key['h_dem']);
}
private function tempsArrêtTemporaireExistant($time)
{
return isset($this->{$Time}['previous_arret']);
}
// [...]
if($this->tempsArrêtEstCalculé())
{
if($this->tempsArrêtTemporaireExistant($Time))
{
//Utilisation de la fonction de calcul qui fait la difference ( voir plus bas )
@$this->{$Time}['t_arret'] += $this->CalculDiff($this->{$Time}['previous_arret'], $key['h_dem']);
}
}<?php
private function tempsArrêtEstCalculé($key)
{
return !is_null($key['h_dem']);
}
private function tempsArrêtTemporaireExistant($time)
{
return isset($this->{$Time}['previous_arret']);
}
private function calculerDifférenceTempsTemporaireEtArrêt($key, $time)
{
@$this->{$time}['t_arret'] += $this->CalculDiff($this->{$time}['previous_arret'], $key['h_dem']);
}
// [...]
if ($this->tempsArrêtEstCalculé())
{
if ($this->tempsArrêtTemporaireExistant($Time))
$this->calculerDifférenceTempsTemporaireEtArrêt($key, $Time);
}Voilà, on a refactoriser toutes nos conditionnelles. On pourrait faire quelques d'autre afin de simplifier ce que nous avons refactorisé. En faitm l'idée c'est de donner un nom à ce que ça fait.
<?php
private function tempsArrêtEstCalculé($key)
{
return !is_null($key['h_dem']);
}
private function tempsArrêtTemporaireExistant($time)
{
return isset($this->{$Time}['previous_arret']);
}
private function calculerDifférenceTempsTemporaireEtArrêt($key, $time)
{
@$this->{$time}['t_arret'] += $this->CalculDiff($this->{$time}['previous_arret'], $key['h_dem']);
}
private function déterminerTempsArrêt($key, $time)
{
if ($this->tempsArrêtEstCalculé())
{
if ($this->tempsArrêtTemporaireExistant($time))
$this->calculerDifférenceTempsTemporaireEtArrêt($key, $time);
}
}
// [...]
$this->déterminerTempsArrêt($key, $Time);Vous comprendrez que le français nous offre aucune ambiguité à ce niveau. Après, si vous êtes bon en anglais, vous êtes libre de faire la même chose en anglais. N'hésitez pas à créer des variables significatives, ce que j'ai omis de faire plus haut. (le franglais c'est pas top... :D)
Bonne chance,
Ramz.