Add a StatementRank class

[thiemowmde]

No description provided.

[JonasKress]

+1

[JeroenDeDauw]

Did you consider having an actual Rank object, and if so, what made you go with the static approach?

[Benestar]

What do we need this for?

[thiemowmde]

This is just a suggestion for a possible enhancement. You can use these alternative constants in a context where it is critical to know that ranks are integers.

[Benestar]

In those cases just use 0 instead of Rank::RANK_0 as you hardcode the zero anyways.

[thiemowmde]

No, what? What is "hardcoded anyway"? I'm not sure you get the point of all this. Constants are for easy tracking and refactoring. A rank should never be expresses via an integer constant.

[Benestar]

Thanks for sharing your proposal, we definitely need an improvement how we handle ranks in our data model. However, I think we could avoid introducing so many methods by just relying on the fact that the constant integers of the rank values replicate the order of the ranks (which is in fact what the class does).

What I think would be more appropriate would be a static method Statement::getAvailableRanks which lets us check if some value is a valid rank. All the comparison stuff can be done using the integer values of the constants.

[JeroenDeDauw]

we definitely need an improvement how we handle ranks in our data model.

Can you elaborate on what you currently find inadequate? The same goes for Thiemos proposal here. It's a lot easier to comment on this if its clear what the goal is.

[thiemowmde]

Did you consider having an actual Rank object

Yes, and I find the idea of an object wrapping nothing but an integer pointless and scary. It also doesn't help solving the issue I want to solve here: The semantics of how ranks are compared is currently duplicated across our code base. This would only be solved if we ditch the idea of ranks being integers completely, across all components. This would be a major breaking change that's not worth it, in my opinion. Therefore: Ranks stay integers and a helper class with some comparison methods is good enough.

just relying on the fact that the constant integers of the rank values replicate the order of the ranks

That's exactly the issue we have: This knowledge is not in a single place but duplicated across the code base. This caused heavy confusion yesterday, but nobody was able to point to that single place that says "ranks are always, now and in the future, guaranteed to be in increasing order from least to most preferred".

which is in fact what the class does

Yes, it does create an explicit place for this knowledge. That's the point.

getAvailableRanks

That's a possible enhancement, but doesn't solve any issue.

All the comparison stuff can be done using the integer values of the constants.

As said above, yes, it could, but no, it shouldn't.

[JeroenDeDauw]

This caused heavy confusion yesterday, but nobody was able to point to that single place that says "ranks are always, now and in the future, guaranteed to be in increasing order from least to most preferred".

That is somewhat surprising to me. Ranks are parts of statements. So if there is no Rank enum, then that is where I'd first look. And if that is too unclear, I'd just add a Rank enum.

[thiemowmde]

How would you do that? There are no enums in PHP.

[JeroenDeDauw]

I meant having a Dedicated class to hold the constants rather than putting them into Statement.

[JeroenDeDauw]

Thinking about it, if we create a dedicated class for ranks, regardless of what we put in there, Rank(s) seems like a bad name. The concept here is not disjoint from statements, so the name ought to reflect that, by being like StatementRank(s).

[thiemowmde]

StatementRank is indeed better, will change.

[Benestar]

This caused heavy confusion yesterday, but nobody was able to point to that single place that says "ranks are always, now and in the future, guaranteed to be in increasing order from least to most preferred".

I wasn't part of that discussion or meeting when this arised so could you summarize what happened? The rank to integer mapping seems to be quite clear to me as there is only one place where the integer values are defined (inside Statement). The only thing that is missing imo is a static place where one can get the list of all available ranks, which can easily be solved by adding a method to Statement. That should btw also answer Jeroen's question.

Can you elaborate on what you currently find inadequate?

We have several places where we hardcode array(deprecated, normal, preferred) in our code and it would be nice if that could be centralized. Beyond that, I don't see the need for another class and therefore more complexity.

[thiemowmde]

could you summarize what happened?

What I just explained: There is no single place that encodes the knowledge what "best" means, when a rank is "better" and so on.

The only thing that is missing imo is a static place where one can get the list of all available ranks

I'm not sure how often I should repeat myself. Please read #562 (comment) again.

[thiemowmde]

We have several places where we hardcode array(deprecated, normal, preferred)

I can not find this array anywhere outside the Statement class. Some tests use arrays of ranks in data providers, but I do not understand how this is a mistake. Tests should be independent from implementations they are not testing.

[thiemowmde]

I'm afraid a method with this signature is of not much use. How would you call it when you have a StatementList?

[JeroenDeDauw]

No changes since 2015. Please reopen if needed for current goals

[JeroenDeDauw]

Please rebase and find reviewers if this is change relevant to your goals. The PR just sitting here with merge conflicts and no one looking at it for years is not helpful.

[JeroenDeDauw]

I like how this provides one place where rank comparisons are defined, though do not think global functions are a good approach. Instead we could encapsulate the primitive, something like

class StatementRank {
publicfunctionisPrefferedOver( self$rank ): bool {}
}

That gets rid of the global functions and follows the primitive encapsulation practice.

[JeroenDeDauw]

Also beware that the @SInCE tags need updating