Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a none function that guarantees an empty set. #80

Closed
wants to merge 1 commit into from
Closed

Added a none function that guarantees an empty set. #80

wants to merge 1 commit into from

Conversation

markchalloner
Copy link

Hi,

I added a function.

ORM::for_table('people')->none()->find_many();

That is guaranteed to return an empty set. This code devolves down to the following query:

SELECT * FROM `person` WHERE 0;

The reasons for having a dedicated function are:

  • Syntatic sugar: none or a similarly named function makes more sense than where_raw(0)
  • Possibility to optimise (by not actually executing the query)

An example use of this code in Paris would be:

class User extends Model {

    public static function has_cookie($orm, $cookie_name) {
        if (isset($_COOKIE[$cookie_name]))  {
            $cookie = $_COOKIE[$cookie_name];
            return $orm->where('cookie', $cookie);
        }
        // Return empty set
        //return $orm->where_raw(0);
        return $orm->none();
    }

}

$users = Model::Factory('user')->filter('has_cookie', 'user')->find_many();

@ghost ghost assigned treffynnon Nov 28, 2012
@treffynnon
Copy link
Collaborator

Thanks for the fresh pull request! As you can see have I targeted this at a 1.3.0 release. :)

@tag
Copy link
Contributor

tag commented Nov 28, 2012

I don't get it. Isn't this effectively a factory, just like create()? Why might none() be used, and not create()? The use case you shared is for Paris ... is there a similar use case for Idiorm, or should this be in the Paris code instead?

@markchalloner
Copy link
Author

The create function, creates a new record in the database. The none function returns an empty record set wrapped in $ORM, effectively a null object (see http://en.wikipedia.org/wiki/Null_Object_pattern)

Null objects are used to separate concerns and remove unnecessary logical branches. This patch is less about the use of Idiorm and more about enabling better app architecture. Example:

Procedurally validating data before checking the database:

$person = ORM::for_table('people');
if (isset($_COOKIE[$cookie_name]))  {
    $cookie = $_COOKIE[$cookie_name];
    if (strlen($name) > 2) {
        $person = $person->where('cookie', $cookie_name)->where('name', $name)->find_one();
    } else {
        $person = false;
    }
} else {
    $person = false;
}

if ($person) {
    // Do something
}

Separating concerns:

function getPersonWithCookie($person, $cookie_name) {
    if (isset($_COOKIE[$cookie_name])) {
        $cookie = $_COOKIE[$cookie_name];
        return $person->where('cookie', $cookie_name)
    }
    return false;
}

function getPersonWithName($person, $name) {
    if (strlen($name) > 2) {
        return $person->where('name', $name);
    }
    return false;
}

function getPerson($name, $cookie) {
    $person = ORM::for_table('people');
    $person = getPersonWithCookie($person, $cookie);
    if ($person) {
        $person = getPersonWithName($person, $name);
        return $person->find_one();
    }
    return $person;
}

$person = getPerson('cookie', 'Alice');
if ($person) {
   // Do something
}

Adding a null object makes getPerson cleaner and lets you separate your fetch from your check:

function getPersonWithCookie($person, $cookie_name) {
    if (isset($_COOKIE[$cookie])) {
        $cookie = $_COOKIE[$cookie_name];
        return $person->where('cookie', $cookie_name)
    }
    return $person->none();
}

function getPersonWithName($person, $name) {
    if (strlen($name) > 2) {
        return $person->where('name', $name);
    }
    return $person->none();
}

function getPerson($person, $cookie, $name)
    $person = ORM::for_table('people');
    $person = getPersonWithCookie($person, $cookie);
    $person = getPersonWithName($person, $name);
    return $person->find_one();
}

$person = getPerson('cookie', 'Alice');
if ($person) {
   // Do something
}

Paris generalises the above (models/chainable methods/filters) in a lot less code, hence the previous example.

Hope that helps.
Mark

@markchalloner
Copy link
Author

Thinking about it create might work too and be cleaner. I'll have a look tomorrow.

Thanks

@tag
Copy link
Contributor

tag commented Nov 29, 2012

Partly, with Paris, I was thinking of simply the Model::factory('className'), which is what I use in similar situations. After digging into Idiorm (but without doing any tests), Idiorm's for_table() might be sufficient for what you're doing, as create() requires an instance, which is what is created by for_table().

Because Idiorm already chains, in the example you provide, each call to return $person->none(); seems as if it could simply be replaced with return $person;.

@markchalloner
Copy link
Author

Whoops, sorry previous comment was me - wrong account:

Not sure that either for_table or create would work correctly though. When _run is called (from find_one or find_many), the SQL used to populate the instance is:

ORM::for_table('people')->find_many();
ORM::for_table('people')->create()->find_many();
SELECT * FROM `people`;
SELECT * FROM `people`;

Which returns all rows, rather than no rows. In the example above, using either for_table (i.e. return person) or create if the name didn't validate or the cookie was present and there was at least one person in the table, // Do something would be run.

The first test in test\test_queries.php has an example of using for_table.

I'm loathe to change this behaviour of _run as it seems to me that the default behaviour of a database query, even wrapped in an ORM should be return all rows (i.e. SELECT * FROM ...).

Thanks

@treffynnon
Copy link
Collaborator

I think I prefer the none method to be honest as it is explicit as well, but Ihave not reviewed the implications yet.

@tag
Copy link
Contributor

tag commented Nov 30, 2012

With your additional comment, I see I misinterpreted the design of your recent example. It's not an architecture I encounter often, thus I'm less comfortable with it; I'm typically much more straightforward in my approach. (And likely because the example is so simplified, I'm still not convinced this approach is the best way to solve this particular problem.)

However, if you and others find a use for this, I'll no longer raise objection. Thanks for putting up with me.

@treffynnon
Copy link
Collaborator

I have thought about this more and I think that returning an empty set in this way isn't any cleaner than just running the query and passing its output into empty(). It adds more complexity.

If I hear no more calls for this feature the ticket will be closed without merge in the new year.

@markchalloner
Copy link
Author

@treffynnon. Slightly confused about your comment. Can you provide some sample code?

@treffynnon
Copy link
Collaborator

Sure, something like this:

$result = ORM::for_table('people')->where('cookie', $cookie_name)->find_many();
if(!empty($result)) {
    // do something with result
}

@markchalloner
Copy link
Author

Got it.

I'm not sure you're comparing apples with apples here though. This is a check for an empty set rather than the ability to deliberately return an empty set.

@treffynnon
Copy link
Collaborator

Yeah, I understand what your pull request is aiming to achieve. I am saying that the additional documentation and complexity can be avoided with defensive programming. So instead of assuming you'll always get a set back from your functions you could return null or false instead. Then you could programming defensively around that.

I agree that it is nice when you can always assume a consistent return type from a method, but I feel that this is something that Idiorm 1 cannot support nicely. So I am leaning towards giving it more thought in a version 2 context so it can be implemented from the very beginning.

@markchalloner
Copy link
Author

OK.

At the risk of being annoying (sorry! 😄), I feel users having to program defensively, especially without documented examples, is less intuitive than a consistent interface requiring additional code/docs.

Programming defensively is definately desirable and to be encouraged, but sometimes user code isn't written that way, sometimes it can't be done (e.g. in the middle of a chain), and it shouldn't just be the domain of user code.

Off my soapbox, happy to drop this with a view to reopening in v2. As a side note, please let me know if you want to discuss v2; architechure interests me.

Thanks

@treffynnon treffynnon closed this Jan 14, 2013
@treffynnon
Copy link
Collaborator

@markchalloner just for interests sake have you seen https://github.com/schmittjoh/php-option

Perhaps that is something that you might find useful.

@markchalloner
Copy link
Author

Thanks @treffynnon, I hadn't, looks interesting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants