Modernize, simplify, refacto the jeedomClass #3101

ColonelMoutarde · 2025-07-03T06:53:10Z

Description

Dans cette PR, je me suis attaché à rendre le code plus explicite et optimal. J'ai aussi essayé de baisser la complexité cyclomatique de certaines méthodes.

Related issues/external references

Improvement

PR checklist

I have checked there is no other PR open for the same change.
I have read the [La ligne directrice pour contribuer à ce projet / Contribution guidelines for this project).
I grant the project the right to include and distribute the code under the GNU.
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.

kwizer15 · 2025-07-05T08:02:25Z

core/class/jeedom.class.php

 	}

-	public static function deadCmd() {
+	public static function deadCmd(): array {


Ce genre de modification, bien que positive à terme, est risqué. Un changement de signature de methode non privée est un breaking change en cas d'extension de cette classe dans un plugin. Bien que peu probable, c'est surement plus prudent de typer via la phpdoc.

core/class/jeedom.class.php

kwizer15 · 2025-07-05T11:32:32Z

core/class/jeedom.class.php

-					sleep(5);
-					$cron->halt();
-				} catch (Error $e) {
+				} catch (Exception|Error $e) {


Cette notation équivaut à catch (Throwable $e) qui me semble plus simple.

kwizer15 · 2025-07-05T11:41:51Z

core/class/jeedom.class.php

+                user::searchByOptions($key),
+                user::searchByRight($key),
+            ];
+            foreach ($searchExpressions as $result) {


plus efficace : $datas = array_merge(...$searchExpressions);

Effectivement, mais le comportement n'est pas le même entre PHP 7.4 et 7.3. pour le moment on devrait laisser comme ça

C'est pas une histoire avec les tableaux vides?
Si c'est ça on peut forcer avec array_merge([], ...$tab);

Pardon d'insister mais le gain de performance sur ce type d'algo est énorme. array_merge est très lent, et dans une boucle on monte vite à plus d'une seconde d'execution.

Je regarde pour tenter d'optimiser.

kwizer15 · 2025-07-05T11:46:12Z

core/class/jeedom.class.php


 	public static function checkSpaceLeft($_dir = null) {
-		if ($_dir == null) {
+		if ($_dir === null) {


Je m’y risquerai pas, sauf si t’as bien étudié les différents appels.

J'ai verifié, c'est ok

pifou25 · 2025-07-06T12:54:03Z

Excellente idée de simplifier / refactoriser les classes :) est-ce qu'on a un dashboard sonarqube qui permettrait de mettre en avant les améliorations possibles ? J'ai essayé de le faire avec snoarcloud.io (gratuit en ligne), par exemple pour cette classe:
https://sonarcloud.io/code?id=pifou25_jeedom-core&selected=pifou25_jeedom-core%3Acore%2Fclass%2Fjeedom.class.php

core/class/jeedom.class.php

ColonelMoutarde · 2025-07-07T09:09:46Z

core/class/jeedom.class.php

+                user::searchByOptions($key),
+                user::searchByRight($key),
+            ];
+            foreach ($searchExpressions as $result) {


Effectivement, mais le comportement n'est pas le même entre PHP 7.4 et 7.3. pour le moment on devrait laisser comme ça

ColonelMoutarde · 2025-07-07T09:11:07Z

core/class/jeedom.class.php


 	public static function checkSpaceLeft($_dir = null) {
-		if ($_dir == null) {
+		if ($_dir === null) {


J'ai verifié, c'est ok

core/class/jeedom.class.php

kwizer15 · 2025-07-07T09:40:24Z

core/class/jeedom.class.php

-					$cron->halt();
-				} catch (Error $e) {
+				} catch (Throwable $e) {
+``


Coquille (merci phpstan)

ColonelMoutarde · 2025-07-07T09:48:57Z

core/class/jeedom.class.php

+                user::searchByOptions($key),
+                user::searchByRight($key),
+            ];
+            foreach ($searchExpressions as $result) {


Je regarde pour tenter d'optimiser.

core/class/jeedom.class.php

l-sanchez-euronews · 2025-07-07T08:41:44Z

core/class/jeedom.class.php

-					sleep(5);
-					$cron->halt();
-				} catch (Error $e) {
+				} catch (Exception|Error $e) {


Suggested change

} catch (Exception|Error $e) {

} catch (\Throwable $e) {

l-sanchez-euronews · 2025-07-08T09:12:20Z

core/class/jeedom.class.php

+                user::searchByOptions($key),
+                user::searchByRight($key),
+            ];
+            foreach ($searchExpressions as $result) {


l-sanchez-euronews · 2025-07-08T12:11:10Z

core/class/jeedom.class.php

 	}

-	public static function deadCmd() {
+	public static function deadCmd(): array {


Suggested change

public static function deadCmd(): array {

/**

* @return array

*/

public static function deadCmd() {

kwizer15 · 2025-07-11T17:41:21Z

core/class/jeedom.class.php

 	}

-	public static function listBackup() {
+	public static function listBackup(): array {


Même chose que pour deadCmd, je voulais pas spammer.
En règle général, un changement de signature de méthode est un BC, sauf cas très particulier.
C’est donc valable pour les fonctions suivantes.

Comme c’est tout de même un changement positif, tu peux éventuellement faire une PR avec uniquement ces changements. Ça permettra de voir si c’est mergeable ou pas.

Et n’hésite pas à compléter les PHP doc, on a fait un petit guide ici https://github.com/jeedom/documentations/pull/157/files (pas encore mergé)

Phpvarious · 2025-07-28T16:19:54Z

Bonjour,
La plupart du temps ce sont des tabulations qui sont faites en début de ligne, hors sur ces modifs ce sont de simples espaces.
Je ne sait pas si c'est un standard pour php, mais pour une meilleure visibilité, surtout sur des fonctions plus complexe, il faudrait garder le même type d'espacement.

kwizer15 · 2025-07-28T16:25:13Z

Bonjour,
La plupart du temps ce sont des tabulations qui sont faites en début de ligne, hors sur ces modifs ce sont de simples espaces.
Je ne sait pas si c'est un standard pour php, mais pour une meilleure visibilité, surtout sur des fonctions plus complexe, il faudrait garder le même type d'espacement.

J'ai déjà fait une PR #3005 dans ce sens (qui date un peu) mais c'est l'idée. Normalement on n'utilise que des espaces en php, pas de tab.

Phpvarious · 2025-07-28T16:38:15Z

J'ai déjà fait une PR #3005 dans ce sens (qui date un peu) mais c'est l'idée. Normalement on n'utilise que des espaces en php, pas de tab.

Il y a du boulot alors :-) , car pratiquement tous les fichiers php utilisent des tab.
Sans doute Github qui par défaut utilise le tabs en édition :

kwizer15 · 2025-08-05T18:40:17Z

Bonjour,
La plupart du temps ce sont des tabulations qui sont faites en début de ligne, hors sur ces modifs ce sont de simples espaces.
Je ne sait pas si c'est un standard pour php, mais pour une meilleure visibilité, surtout sur des fonctions plus complexe, il faudrait garder le même type d'espacement.

J'ai déjà fait une PR #3005 dans ce sens (qui date un peu) mais c'est l'idée. Normalement on n'utilise que des espaces en php, pas de tab.

Je peux accepter sans problème qu'on n'ai des points de vue différents, mais encore faudrait-il expliquer son point de vue plutôt que de mettre un pouce vers le bas sans autre explication @Salvialf

Update jeedom.class.php

ddc28e8

ColonelMoutarde marked this pull request as draft July 3, 2025 06:55

Update jeedom.class.php

e9a2190

ColonelMoutarde marked this pull request as ready for review July 3, 2025 07:36

kwizer15 reviewed Jul 5, 2025

View reviewed changes

Mips2648 self-requested a review July 5, 2025 09:23

kwizer15 reviewed Jul 5, 2025

View reviewed changes

ColonelMoutarde commented Jul 7, 2025

View reviewed changes

Apply suggestions from code review

ed17326

kwizer15 reviewed Jul 7, 2025

View reviewed changes

Update jeedom.class.php

aecd435

ColonelMoutarde commented Jul 7, 2025

View reviewed changes

ColonelMoutarde added 3 commits July 7, 2025 11:52

Update core/class/jeedom.class.php

d62b75a

oups

76eec24

Optimize replaceTag()

eb028a6

l-sanchez-euronews reviewed Jul 8, 2025

View reviewed changes

l-sanchez-euronews reviewed Jul 9, 2025

View reviewed changes

kwizer15 reviewed Jul 11, 2025

View reviewed changes

Hotfirenet requested review from Hotfirenet, Salvialf, Sekiro-kost, kwizer15, l-sanchez-euronews, reineabs, tmartinez69009 and zoic21 July 30, 2025 06:41

l-sanchez-euronews approved these changes Aug 5, 2025

View reviewed changes

Modernize, simplify, refacto the jeedomClass #3101

Are you sure you want to change the base?

Modernize, simplify, refacto the jeedomClass #3101

Conversation

ColonelMoutarde commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues/external references

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwizer15 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pifou25 commented Jul 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Phpvarious commented Jul 28, 2025

Uh oh!

kwizer15 commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Phpvarious commented Jul 28, 2025

Uh oh!

kwizer15 commented Aug 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ColonelMoutarde commented Jul 3, 2025 •

edited

Loading

kwizer15 Jul 7, 2025 •

edited

Loading

kwizer15 commented Jul 28, 2025 •

edited

Loading