Skip to content

add ResultLimit feature, implements ideas from #29 #57

Merged
merged 8 commits into from May 1, 2012

3 participants

@benib
benib commented Apr 9, 2012

this implements some things discussed in issue #29.
default is to return all fields. if fields parameter is set, only the defined fields are returned as long as there is a check in the code, where the field is added to the result. this can be done like:

if (ResultLimit::includeField('from')) {
    // add from stuff to result
}

these checks are implemented in Transport\Entity\Schedule\Connection for the fields "from", "to" and "journey" to show how this works.

Tree like fields are also possible, eg:

http://sbb.local/v1/connections?from=008501120&to=008501008&fields[]=to&fields[]=from/geo

in that case, ResultLimit::includeField('from') returnes true, as there is a more specific field set.

any comments are appreciated.

@rndstr
opendata.ch member
rndstr commented Apr 10, 2012

Quick feedback:

  • Why not make everything static in ResultLimit, get rid of the Singleton, and add a private ctor? Although I would prefer seeing it as an instance passed around or even better pass the $request somehow. Haven't yet thought it through, though.
  • You should put open parentheses of classes and methods on their own line (see rest of codebase)
  • Tests? ;)
@benib
benib commented Apr 10, 2012
  • i agree about making everything static.
  • also agree about coding styles
  • personally i like the approach with the static function includeField(), that way, we can just add this call where some data is added to the request and don't have to worry about passing instances around. the information which fields to include is static and only set once with the request sent to the API.
@rndstr rndstr commented on an outdated diff Apr 10, 2012
lib/Transport/ResultLimit.php
+<?php
+
+namespace Transport;
+
+/**
+ * ResultLimit
+ */
+class ResultLimit
+{
+ private static $fields = null;
+
+ private function __construct()
+ {
+ }
+
+ public static function setFields($fields)
@rndstr
opendata.ch member
rndstr added a note Apr 10, 2012

I suggest doing type hinting here array $fields instead of the is_array() check below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rndstr rndstr commented on an outdated diff Apr 10, 2012
lib/Transport/ResultLimit.php
+
+ private function __construct()
+ {
+ }
+
+ public static function setFields($fields)
+ {
+ if (is_array($fields)) {
+ self::$fields = array();
+ foreach ($fields as $field) {
+ self::$fields = array_merge(self::$fields,self::getFieldTree($field));
+ }
+ }
+ }
+
+ public static function includeField($field, $searchBase = null)
@rndstr
opendata.ch member
rndstr added a note Apr 10, 2012

phpdoc would be nice :)

Edit: And I think isFieldIncluded() would be a better name since includeField() sounds like an operation while it is a query

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rndstr rndstr commented on an outdated diff Apr 10, 2012
lib/Transport/ResultLimit.php
+ * ResultLimit
+ */
+class ResultLimit
+{
+ private static $fields = null;
+
+ private function __construct()
+ {
+ }
+
+ public static function setFields($fields)
+ {
+ if (is_array($fields)) {
+ self::$fields = array();
+ foreach ($fields as $field) {
+ self::$fields = array_merge(self::$fields,self::getFieldTree($field));
@rndstr
opendata.ch member
rndstr added a note Apr 10, 2012

not to be too pedantic but could you add a whitespace after the comma? (same for the occurences below)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rndstr rndstr commented on an outdated diff Apr 10, 2012
lib/Transport/ResultLimit.php
+ } else {
+ foreach ($searchBase as $newSearchBase) {
+ if (is_array($newSearchBase)) {
+ $result = self::includeField($field,$newSearchBase);
+ }
+ //if the field is found, return this information to stop crawling at this point
+ if ($result) {
+ return $result;
+ }
+ }
+ //the field doesn't seem to be set
+ return false;
+ }
+ }
+
+ private static function getFieldTree($field)
@rndstr
opendata.ch member
rndstr added a note Apr 10, 2012

This method can be rewritten as:

private static function getFieldTree($field) 
{
    return array_reduce(
        array_reverse(explode('/', $field)),
        function ($result, $value) { return array($value => $result); },
        true
    );
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@benib
benib commented Apr 10, 2012

@rndstr: i sticked to isFieldSet as isFieldIncluded seems to be confusing to me, as the field is not yet included, we want to know if it should be or not.

@rndstr
opendata.ch member
rndstr commented Apr 10, 2012

Thanks for the changes! I'm okay with ResultLimit being static for now I guess.

But looking at the code, I see you are only checking for the from, to and journey parts in the Connection resource. What is your idea about the other resources and their fields?

@benib
benib commented Apr 11, 2012
  • checks are now implemented for /connections and /stationboard
  • if no fields are set, everything is returned
  • if any field is set, only the specified fields are returned
  • fields are specified in a path like model
  • if eg. connections/from is set, only the from element and everything below is returned
  • if eg. connections/from/station is set, only the from element containing only the station is returned
  • this can be combined to something like fields[]=connections/from/station&fields[]=connections/to/arrival
@fabian fabian merged commit 43e97eb into OpendataCH:master May 1, 2012
@fabian
opendata.ch member
fabian commented May 1, 2012

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.