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

Question about LCOM4 with Singleton-likes classes #262

Open
niconoe- opened this issue Feb 16, 2017 · 7 comments
Open

Question about LCOM4 with Singleton-likes classes #262

niconoe- opened this issue Feb 16, 2017 · 7 comments
Labels

Comments

@niconoe-
Copy link
Contributor

Hi, still me 😄 ,

I have a question about the LCOM4 value used and even after reseach I wasn't able to find any document or explaination.
Basically, the LCOM4 is used to calculate how many responsabilities a class have.

Now, I have a class that looks like a singleton (but it's NOT a singleton) in its structure and it should have only 1 responsability.
Here is my class:

<?php

class Foo
{
    protected $a;
    protected $b;

    public static function build($a, $b)
    {
        return new self($a, $b);
    }

    protected function __construct($a, $b)
    {
        $this->a = $a;
        $this->b = $b;
    }

    // Only accessors below, it does not matter.
    //...
}

Using both PhpMetrics 1.10.0 and PhpMetrics 2.0, I can see an LCOM4 = 2 for this kind of class.

My question is: "Is LCOM4 = 2 the expected result?".

This is obvious that this class only have 1 responsability, which is build an object with it's properties. But the counter says "2" because Foo::build is related to nothing (nbWorkflow = 1) and the constructor (with all its accessors) is related to all properties (nbWorkflow = 2).

So, what is the position of the "creators" of the LCOM4 method about this? Does static methods must be ignored? In this case, a class with 6 static methods doing 6 unrelated stuffs (understand the class owns 6 responsabilities) should have an LCOM4 = 0, which is also incorrect?

Maybe because of the presence of new self in the Foo::build method, we can add the link between Foo::build and Foo::__construct, but how can we manage inheritance in this case?

I hope you understood what I wanted to explain with this issue, and if you want, I can make the same thread in French for a better understanding between us about this.
Thank you for your great tool.

Regards,

@Halleck45
Copy link
Collaborator

Halleck45 commented Apr 10, 2017

I'm really sorry, but I'll speack french here. Do not hesitate to translate the thread if you're motivated...

Bonjour @niconoe- , content de te voir ici :)

C'est une bonne question, à la quelle je ne sais pas répondre. Je vais me pencher sur le sujet, mais je pense pouvoir dire que cette métrique ne s'applique pas aux objets sans état (la méthode de la factory est ici statique).

  1. Si la méthode build() reste statique, il me paraît pertinent que LCOM = 2.
  2. Si la méthode build() ne l'était pas, en effet, j'aurai tendance à dire que LCOM = 1.

En attendant de trouver une réponse plus définitive, penses-tu pouvoir faire une PR sur le sujet ? Si oui je la mergerai.

Merci de ton retour ! (et désolé pour la latence)

@niconoe-
Copy link
Contributor Author

niconoe- commented Apr 10, 2017

This is the translation of the @Halleck45 previous comment. If you understood it, you can skip this comment.

Hi @niconoe- , glad to see you here :)

That is a good question, in which I can't answer. I'll study the topic, but I think I can tell you that metric doesn't apply to stateless objects (the factory's method here is static).

  1. If the build() method stay static, it seems relevent to me that LCOM = 2.
  2. If the build() method was not, indeed, I should say LCOM = 1.

Before being absolutely sure about this answer, do you think you can make a PR on this subject? In this case, I'll merge it.

Thank you for being back! (and sorry about the delay)

@niconoe-
Copy link
Contributor Author

Français
Salut @Halleck45, et merci de ton avis sur cette question épineuse ;).

D'après ta réponse, j'ai du mal à saisir pourquoi dans le cas où build() reste statique, le LCOM = 2.
En effet, si cette métrique ne s'applique pas aux objets sans état, il me paraît plus cohérent que la méthode doit être ignorée, et le nombre de workflow possible est réduit à 1 : workflow qui ne contient que le constructeur.

Dans ce contexte d'objet sans état, ta réponse m'a rappelé un élément que j'avais oublié au sujet des constructeurs : ils sont eux-mêmes implicitement statiques (comme on peut l'apercevoir lorsqu'on appelle parent::__construct()). Peut-être que les constructeurs (et destructeurs, par opposition) doivent également être ignorés ?
D'ailleurs, on peut penser d'une classe ne contenant que des propriétés, un constructeur et des accesseurs qu'elle ressemble à une simple structure en C et ne possède donc aucune responsabilité, non ? Bref, cela serait plutôt à discuter dans une autre issue, mais je vais me documenter un peu plus avant de l'ouvrir.

J'ai un peu trop d'incertitudes pour te faire une PR à ce sujet, comme tu peux le voir ici ;). Mais dès que ces voiles noirs seront levés, ce sera avec plaisir ! ;).

Merci encore.
Cordialement,


English
Hi @Halleck45, and thank you for answering about this hard question ;).

Based on your previous comment, I barely know why, in case build() stays static, the LCOM = 2.
Actually, if this metric doesn't apply on stateless objects, it seems more relevent to me that this method should be ignored, and thus, the number of possible workflow is reduced to 1: workflow containing only the constructor.

In this context of stateless objects, your answer reminds me an element I forgot about constructors: they're implicitly static (as we can guess when we call parent::__construct()). Maybe those constrictors (and destructors by opposition) must also be ignored?
In addition, we may think a simple class containing only properties, constructors and accessors, it looks like a struct in C and so, owns no responsability, isn't it? Anyway, this would belong to another thread, but I'll read some documentation before opening it.

I'm too much incertain to make you a PR about this issue, as you can read here ;). But as soon as we'll get final answers, it will be with pleasure! ;).

Thanks again.
Regards,

@Halleck45
Copy link
Collaborator

@niconoe- Petite typo dans mon message :( . J'ai édité ; je voulais dire :

Si la méthode build() reste statique, il me paraît pertinent que LCOM = 2.
Si la méthode build() ne l'était pas, en effet, j'aurai tendance à dire que LCOM = 1.

@niconoe-
Copy link
Contributor Author

Ah ah, très bien, je saisis mieux ;).

Donc effectivement, pour le build() dynamique, je suis OK avec toi. Je peux travailler sur une PR pour relier les méthodes dynamiques aux constructeurs si elles contiennent :

  • new self
  • new static
  • new __CLASS__
  • new {nom_de_la_classe}

Est-ce que tu vois une autre façon d'instancier (l'appel explicite à __construct étant, il me semble, déjà automatiquement géré par ta façon de lier les méthodes entre-elles) ?

Je te laisse approfondir tes recherches et tes réflexions pour le cas du build statique. Personnellement, je trouve que le build est tout de même lié au constructeur, qu'elle soit statique ou non, peu importe si la métrique n'est pas lié aux objets sans états ;).

@Halleck45
Copy link
Collaborator

@niconoe can I close this issue ?

@niconoe-
Copy link
Contributor Author

Well, I did not provide a PR to make it, but there is certainly something wrong in the way we calculate the LCOM4 in this case. Considering a method calling a new self and the constructor in 2 different graph is wrong, and there is something to do to fix it. The main problem is, I don't know how to make that connection, and I don't have time to understand the code to make it currently.

You could close this one if you open another (maybe in English) explaining this status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants