Introduce UserNote DTO

[kamil-tekiela]

It would be great if someone could retest it before approving. I tested it myself and saw no regressions.

This PR solves a number of code issues. Mostly it removes nonsense in the Sorter class.

[localheinz]

How do you feel about marking these as private and adding accessors instead?

Suggested change

	/** @var string $id */
	public $id;
	/** @var string $sect */
	public $sect;
	/** @var string $rate */
	public $rate;
	/** @var string $ts */
	public $ts;
	/** @var string $user */
	public $user;
	/** @var string $text */
	public $text;
	/** @var int $upvotes */
	public $upvotes;
	/** @var int $downvotes */
	public $downvotes;
	- /** @var string $id */
	- public $id;
	- /** @var string $sect */
	- public $sect;
	- /** @var string $rate */
	- public $rate;
	- /** @var string $ts */
	- public $ts;
	- /** @var string $user */
	- public $user;
	- /** @var string $text */
	- public $text;
	- /** @var int $upvotes */
	- public $upvotes;
	- /** @var int $downvotes */
	- public $downvotes;
	+ private $id;
	+ private $sect;
	+ private $rate;
	+ private $ts;
	+ private $user;
	+ private $text;
	+ private $upvotes;
	+ private $downvotes;

[cmb69]

I'd rather go with readonly (as annotation for now), if the properties are actually readonly.

[localheinz]

@cmb69

I think they are mutated currently.

[kamil-tekiela]

I experimented with few different approaches. In the end, I decided that it doesn't have to be immutable. With PHP 8.1, we might easily convert this into readonly and use constructor property promotion. It would make migration easier if we use public properties now. I'd keep it as it is now, optionally adding readonly annotation as cmb69 suggests.

[kamil-tekiela]

Thinking about it more, I decided to mark the class as immutable and readonly. I don't think any static analyser respects readonly annotation on class, but if we migrate to PHP 8.2, we will know to make the class as readonly.

[localheinz]

👍

[cmb69]

Would it make sense to bring the UserNotes\Sorter under test before doing this? I mean we would need to modify the test as well, but still could have more confidence to not inadvertently break anything.

Maybe something like the following PHPT:

--TEST--

--FILE--
<?php
require_once __DIR__ . "/../src/UserNotes/Sorter.php";

$notes = [
    [
        "votes" => ["up" => 2, "down" => 0],
        "xwhen" => 1613487094,
    ], [
        "votes" => ["up" => 0, "down" => 0],
        "xwhen" => 1508180150,
    ], [
        "votes" => ["up" => 14, "down" => 3],
        "xwhen" => 1508179844,
    ],
];

$sorter = new phpweb\UserNotes\Sorter();
$sorter->sort($notes);
var_dump($notes);
?>
--EXPECT--
array(3) {
  [2]=>
  array(6) {
    ["votes"]=>
    array(2) {
      ["up"]=>
      int(14)
      ["down"]=>
      int(3)
    }
    ["xwhen"]=>
    int(1508179844)
    ["score"]=>
    int(11)
    ["total"]=>
    int(17)
    ["rating"]=>
    float(0.82352941176471)
    ["sort"]=>
    float(87.411764705882)
  }
  [0]=>
  array(6) {
    ["votes"]=>
    array(2) {
      ["up"]=>
      int(2)
      ["down"]=>
      int(0)
    }
    ["xwhen"]=>
    int(1613487094)
    ["score"]=>
    int(2)
    ["total"]=>
    int(2)
    ["rating"]=>
    int(1)
    ["sort"]=>
    float(48.236363636364)
  }
  [1]=>
  array(6) {
    ["votes"]=>
    array(2) {
      ["up"]=>
      int(0)
      ["down"]=>
      int(0)
    }
    ["xwhen"]=>
    int(1508180150)
    ["score"]=>
    int(0)
    ["total"]=>
    int(0)
    ["rating"]=>
    float(0.5)
    ["sort"]=>
    float(41.400005811566)
  }
}

[kamil-tekiela]

Yes, it would make sense. The problem with this is that I couldn't understand what the current implementation does, which is why I rewrote and simplified it. So while a test would be nice to prevent regression, I don't actually know what would make a good test to make sure we catch the edge cases. If you are ok with a simple test like the one proposed above, I am ok to wait for a test before merging this PR. I have checked the note order before and after myself, but I still can't guarantee that all edge cases are covered.

[localheinz]

@cmb69 @kamil-tekiela

In regard to tests, see #605!

[cmb69]

See also #606.

[kamil-tekiela]

@cmb69 Do we need to adjust the tests more?

[localheinz]

@kamil-tekiela

Can you rebase on top of master?

[cmb69]

Do we need to adjust the tests more?

If we can't show the internals (especially score, rating and sort), we should have more tests. I've written the tests assuming that we still can verify these internals, and so didn't look for testing the final sort order thoroughly. And even if the current sort order might not be the best, I think we should stick with it when refactoring.

[kamil-tekiela]

@cmb69 I added a regression test which takes the sample list of 46 notes and sorts them. The expected output is the same as is available on master, so if the expected sort and new sort is the same, it's safe to assume the logic hasn't been broken. There's still unlikely chance that some edge case was missed, but I have no idea how to test it more thoroughly than this.

[cmb69]

This looks good to me. Thank you!