Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Random network check #3021

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Random network check #3021

merged 2 commits into from
Feb 5, 2025

Conversation

zoic21
Copy link
Contributor

@zoic21 zoic21 commented Jan 31, 2025

La vérification de la connexion réseau est désormais effectuée de manière aléatoire toutes les 10 minutes afin d'éviter que toutes les box Jeedom ne tentent de se reconnecter simultanément en cas de microcoupure.

@@ -379,6 +379,25 @@
$cron->setTimeout(10);
$cron->save();

$cron = cron::byClassAndFunction('network', 'cron10');
if (!is_object($cron)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je sais que le reste est fait de la même façon, mais ca serait surement plus précis de faire un if (!$cron instanceof cron). Qu'en penses-tu ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C’est une optimisation sans réel intérêt, car cron::byClassAndFunction('network', 'cron10') retourne déjà soit une instance de la classe cron, soit null si aucune instance n’est trouvée.

Dans ce cas, if (!is_object($cron)) et if (!$cron instanceof cron) sont redondants, car la méthode statique renvoie directement une instance de cron ou null. La vérification if (!$cron instanceof cron) n'apporte donc rien de plus que if (!is_object($cron)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je tiens à préciser que ma suggestion ne porte pas sur l'optimisation mais sur la qualité et la maintenabilité du code.

L'utilisation de is_object() fonctionne techniquement, mais on pourrait communiquer plus clairement l'intention du code. Il y a plusieurs aspects à considérer :

  1. Typage explicite : instanceof cron ou === null documente directement dans le code le type attendu. Un nouveau développeur n'a pas besoin de consulter la documentation de byClassAndFunction() pour comprendre ce qui est retourné.

  2. Protection contre les changements : Si demain la méthode byClassAndFunction() est modifiée (cas très peu probable, mais possible) pour retourner autre chose qu'un objet cron (par exemple une stdClass contenant des données de cron), is_object() ne détectera pas ce changement qui pourrait introduire des bugs subtils.

  3. Contrat de méthode : Si la méthode retourne strictement cron|null, alors if (null === $cron) serait la solution la plus expressive. Elle indique clairement qu'on vérifie l'absence d'un cron pour le créer ensuite.

Ces pratiques s'alignent avec les principes SOLID, particulièrement le principe de substitution de Liskov, en rendant explicite les types attendus dans le code.

Copy link
Contributor

@Salvialf Salvialf Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonjour,

Merci pour la leçon mais en l'occurrence c'est bien un objet qui est attendu ici.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Un dernier point sur le sujet et je vous laisse tranquille :)

J'ai vu passer beaucoup de projets, je maintiens que la différence entre un code "qui fonctionne" et un code "professionnel" réside souvent dans ces petits détails de typage explicite et d'intentions claires.

Ce n'est pas une question d'optimisation ou de fonctionnement - ce code marche très bien. C'est une question de maintenabilité et de robustesse sur le long terme. Ces petites attentions font toute la différence quand on reprend le code 6 mois plus tard.

Bref, ce n'était qu'une suggestion basée sur mon expérience. Je sais que vous laisserez le code en l'état et ce n'est pas grave. Toutefois, un peu plus de professionalisme dans vos réponses seraient le bienvenue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis tout à fait d’accord avec toi. L’objectif est d’échanger et de trouver le meilleur code possible. Ces détails de typage et de clarté sont importants pour la maintenabilité et la robustesse sur le long terme.

L’échange reste ouvert, et l’essentiel est d’avancer ensemble vers une solution optimale.

Copy link
Contributor

@Hotfirenet Hotfirenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suite correction du texte

@zoic21 zoic21 merged commit 8430545 into alpha Feb 5, 2025
6 checks passed
@zoic21 zoic21 deleted the patch-1 branch February 5, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants