Add a StatementRank class #562

thiemowmde · 2015-10-08T15:04:26Z

No description provided.

JonasKress · 2015-10-08T15:10:43Z

+1

JeroenDeDauw · 2015-10-08T19:49:37Z

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

Benestar · 2015-10-09T09:37:25Z

src/Statement/Rank.php

+	 */
+	const RANK_0 = self::DEPRECATED;
+	const RANK_1 = self::NORMAL;
+	const RANK_2 = self::PREFERRED;


What do we need this for?

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.

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

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 · 2015-10-09T09:40:29Z

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 · 2015-10-09T10:31:38Z

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 · 2015-10-09T10:39:23Z

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 · 2015-10-09T15:23:34Z

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 · 2015-10-09T16:15:57Z

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

JeroenDeDauw · 2015-10-09T16:22:15Z

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

JeroenDeDauw · 2015-10-09T16:24:07Z

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 · 2015-10-09T16:24:51Z

StatementRank is indeed better, will change.

Benestar · 2015-10-09T18:52:09Z

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 · 2015-10-12T12:10:32Z

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 · 2015-10-12T12:16:24Z

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 · 2016-03-08T11:13:53Z

src/Statement/StatementRank.php

+	 *
+	 * @return int|null Best rank in the array or list of arguments, or null if none given.
+	 */
+	public static function findBestRank( $ranks = array() /*...*/ ) {


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

JeroenDeDauw · 2018-08-08T15:05:50Z

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

JeroenDeDauw · 2018-09-17T13:24:09Z

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 {
    public function isPrefferedOver( self $rank ): bool {}
}

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

JeroenDeDauw · 2018-09-17T14:11:32Z

Also beware that the @SInCE tags need updating

thiemowmde added the enhancement label Oct 8, 2015

Benestar reviewed Oct 9, 2015
View reviewed changes

thiemowmde force-pushed the rankClass branch 4 times, most recently from 8f22d3c to fbc9bcf Compare October 15, 2015 13:40

thiemowmde changed the title ~~[WIP] Add a Rank class~~ Add a StatementRank class Oct 15, 2015

thiemowmde force-pushed the rankClass branch from fbc9bcf to 9671d7e Compare November 10, 2015 11:18

thiemowmde reviewed Mar 8, 2016
View reviewed changes

thiemowmde force-pushed the rankClass branch from 9671d7e to 1b9cf48 Compare June 29, 2016 09:24

JeroenDeDauw closed this Aug 8, 2018

JeroenDeDauw deleted the rankClass branch August 8, 2018 15:05

thiemowmde restored the rankClass branch August 13, 2018 11:28

thiemowmde reopened this Aug 13, 2018

JeroenDeDauw closed this Sep 17, 2018

JeroenDeDauw deleted the rankClass branch September 17, 2018 13:24

thiemowmde restored the rankClass branch September 17, 2018 13:43

thiemowmde reopened this Sep 17, 2018

Add a StatementRank class

7f76c7f

thiemowmde force-pushed the rankClass branch from 1b9cf48 to 7f76c7f Compare September 17, 2018 13:59

JeroenDeDauw suggested changes Sep 17, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a StatementRank class #562

Add a StatementRank class #562

thiemowmde commented Oct 8, 2015 •

edited

Loading

JonasKress commented Oct 8, 2015

JeroenDeDauw commented Oct 8, 2015

Benestar Oct 9, 2015

thiemowmde Oct 9, 2015

Benestar Oct 9, 2015

thiemowmde Oct 12, 2015

Benestar commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

thiemowmde commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

thiemowmde commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

thiemowmde commented Oct 9, 2015

Benestar commented Oct 9, 2015

thiemowmde commented Oct 12, 2015

thiemowmde commented Oct 12, 2015

thiemowmde Mar 8, 2016

JeroenDeDauw commented Aug 8, 2018

JeroenDeDauw commented Sep 17, 2018

JeroenDeDauw left a comment

JeroenDeDauw commented Sep 17, 2018

Add a StatementRank class #562

Are you sure you want to change the base?

Add a StatementRank class #562

Conversation

thiemowmde commented Oct 8, 2015 • edited Loading

JonasKress commented Oct 8, 2015

JeroenDeDauw commented Oct 8, 2015

Benestar Oct 9, 2015

Choose a reason for hiding this comment

thiemowmde Oct 9, 2015

Choose a reason for hiding this comment

Benestar Oct 9, 2015

Choose a reason for hiding this comment

thiemowmde Oct 12, 2015

Choose a reason for hiding this comment

Benestar commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

thiemowmde commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

thiemowmde commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

JeroenDeDauw commented Oct 9, 2015

thiemowmde commented Oct 9, 2015

Benestar commented Oct 9, 2015

thiemowmde commented Oct 12, 2015

thiemowmde commented Oct 12, 2015

thiemowmde Mar 8, 2016

Choose a reason for hiding this comment

JeroenDeDauw commented Aug 8, 2018

JeroenDeDauw commented Sep 17, 2018

JeroenDeDauw left a comment

Choose a reason for hiding this comment

JeroenDeDauw commented Sep 17, 2018

thiemowmde commented Oct 8, 2015 •

edited

Loading