Add user statistics #3
Conversation
src/Users/UserStatistics.php
Outdated
| */ | ||
| class UserStatistics extends Model | ||
| { | ||
| /** |
There was a problem hiding this comment.
Remove getStatistics() in favor of get()
src/Users/UserStatistics.php
Outdated
| * | ||
| * @return Statistic. | ||
| */ | ||
| public function getStatistic($typeCode,$categoryCode) |
There was a problem hiding this comment.
Confirm whether multiple Statistics can really exist across a single type/category combination.
Also, compare with similar method signatures in codetables, identifiers, etc. In codetables, for example, there is a pattern of:
// get a single entry
$client->codetable->get( $code );
// get all:
$client->codetable->getCodeTables( );
Is this typical? If so, should this look like:
// get one
$user->Statistics->get( $type, $category );
// get all
$user->Statistics->getAll( );
src/Users/UserStatistics.php
Outdated
| * @param string $note free text description of the statistic. | ||
| * @param string $segment_type either "Internal" or "External" | ||
| */ | ||
| public function addStatisticRaw($typeCode,$typeDesc,$categoryCode,$categoryDesc,$segment_type = 'External' ,$note = '') |
There was a problem hiding this comment.
Suggested refactor:
addStatistic() adds the values unchecked (in line with the fact that such invalid data can exist in Alma
But add a Statistics->Statistic->validate() method, and Statistics->validate() method to confirm the statistics against actual codetables.
Warning: this has substantial implications for the Lazy Load approach, and for caching of codetables.
src/Users/UserStatistics.php
Outdated
| * @param string $segmentType either ("Internal" or "External") | ||
| * @param string $note | ||
| */ | ||
| public function addStatistic($typeCode,$categoryCode,$segmentType = 'External',$note = '') |
There was a problem hiding this comment.
In the context of Statistics, since it looks like we will be filling out the Statistic class as well, an add() method should probably take a Statistic object, rather than a set of parameters.
src/Users/UserStatistics.php
Outdated
| */ | ||
| public function removeAllStatistics() | ||
| { | ||
| foreach($this->data->user_statistic as $key => $row) { |
There was a problem hiding this comment.
This should simply be:
$this->data-user_statistic = array();
src/Users/UserStatistics.php
Outdated
| * @param string $segmentType ("Internal" or "External") | ||
| * @param string $note | ||
| */ | ||
| public function updateStatistic($fromTypeCode,$fromCategoryCode,$toTypeCode,$toCategoryCode,$segmentType = "External" ,$note = '') |
There was a problem hiding this comment.
The update method is better as a member of Statistic rather than Statistics.
src/Users/UserStatistics.php
Outdated
| * @param string $typeCode | ||
| * @return Array of Statistics. | ||
| */ | ||
| public function getStatsByTypeCode($typeCode) |
There was a problem hiding this comment.
Lots of helper methods here, mostly performing various formulations of array searches. These can be attractive in an SDK, but one argument against them is that if we write them, we are also responsible to write tests for them.
No description provided.