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

Model instance isPersisted() and propertyIsBlank() methods #559

Closed
chrisdpeters opened this Issue Oct 15, 2015 · 10 comments

Comments

3 participants
@chrisdpeters
Contributor

chrisdpeters commented Oct 15, 2015

Rails has #persisted?, which is the inverse of #new_record? to help avoid "negative" boolean logic.

For example, if user.persisted? is easier to process than if !user.new_record? or unless user.new_record?.

I propose that we add isPersisted() for the same reason.

If there are no objections, I'd be glad to implement and document this for CFWheels 2.0.

@chrisdpeters chrisdpeters added this to the 2.0.0 milestone Oct 15, 2015

@chrisdpeters chrisdpeters self-assigned this Oct 15, 2015

@chrisdpeters

This comment has been minimized.

Contributor

chrisdpeters commented Oct 21, 2015

@perdjurner

This comment has been minimized.

Contributor

perdjurner commented Oct 21, 2015

Sounds like a good improvement.

Maybe we should take this opportunity to review other similar functions?

exists()
hasChanged()
hasErrors()
hasProperty()
isAjax()
isClass()
isGet()
isInstance()
isNew()
isPost()
isSecure()
propertyIsPresent()
valid()

@perdjurner

This comment has been minimized.

Contributor

perdjurner commented Oct 21, 2015

To follow up...

isClass() and isInstance() are already the opposites of each other so we can remove those from the list.

I think we can also remove isGet(), isAjax(), isPost() from the list to review since I think it's rare that you would want to use the oppose of any of those.

This leaves us with:

exists()
hasChanged()
hasErrors()
hasProperty()
isNew()
isSecure()
propertyIsPresent()
valid()

@chrisdpeters

This comment has been minimized.

Contributor

chrisdpeters commented Oct 21, 2015

Interesting idea.

This may be a good time to come up with an alias for valid too. I understand why it can't be called isValid because of the built-in CF function, but it's the only one that doesn't read as a boolean.

passesValidation -> failsValidation

Many of those I cannot think of antonyms for, which could be a challenge.

exists -> ?
hasChanged -> hasUnchanged
hasErrors -> ?
hasProperty -> ?
isNew -> isPersisted
isSecure -> isInsecure
propertyIsPresent -> propertyIsBlank
valid/passesValidation -> failsValidation

@perdjurner

This comment has been minimized.

Contributor

perdjurner commented Oct 22, 2015

Are you avoiding using the word "not" for a specific reason?

If not, wouldn't:

hasChanged -> hasNotChanged

be easier to read than:

hasChanged -> hasUnchanged

@chrisdpeters

This comment has been minimized.

Contributor

chrisdpeters commented Oct 22, 2015

Yes, I am. I'd hate to see code that looks like this:

<cfscript>
if (!this.hasNotChanged()) {
}
</cfscript>

Or even better:

<cfif not this.hasNotChanged()>
</cfif>

Am I being too pessimistic?

@perdjurner

This comment has been minimized.

Contributor

perdjurner commented Oct 22, 2015

I do think you are being too pessimistic here.

We should provide the best options for creating readable code, and do our best to use it in documentation so developers can pick up on it.

In that regard I prefer something.hasNotChanged() over something.hasUnChanged().

@neokoenig

This comment has been minimized.

Member

neokoenig commented Oct 22, 2015

We need to be careful with double negatives.

Good:
if ( this.hasChanged() ){} or if ( ! this.hasChanged() ){}

Not good:

if (! this.hasNotChanged() ){}

"unChanged" I don't think is a clear term.

I'm definitely more of a fan of is statements being positive assertions, and using a ! or not operator . Personally, that's clearer to me, but there you go.

@chrisdpeters

This comment has been minimized.

Contributor

chrisdpeters commented Oct 23, 2015

Rails doesn't implement any of these other than the equivalent to what would be our isPersisted() and propertyIsBlank().

Though I'm glad we took a minute to explore it, I feel like the other ideas are forced. Including the word "Not" in the name probably isn't any easier to parse mentally than the ! or not boolean operators when reading the code as a developer.

@perdjurner perdjurner modified the milestone: 2.0.0 Mar 30, 2016

@perdjurner perdjurner removed the roadmap label Mar 30, 2016

@chrisdpeters chrisdpeters changed the title from Model instance `isPersisted()` method to Model instance `isPersisted()` and `propertyIsBlank()` methods Dec 2, 2016

@chrisdpeters chrisdpeters changed the title from Model instance `isPersisted()` and `propertyIsBlank()` methods to Model instance isPersisted() and propertyIsBlank() methods Dec 2, 2016

chrisdpeters added a commit that referenced this issue Dec 2, 2016

@chrisdpeters

This comment has been minimized.

Contributor

chrisdpeters commented Dec 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment