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

Use attr_accesor when posible #9

Closed
clbustos opened this Issue Jul 7, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@clbustos

clbustos commented Jul 7, 2013

In many classes, you use methods like

def name; @name; end

In Ruby, Is conventional to replace all getters and setters with
attr_reader :name
attr_writter :name

So, please use the first form and document it.

@zuhao

This comment has been minimized.

Show comment
Hide comment
@zuhao

zuhao Jul 8, 2013

Owner

Yes I am aware of attr_*, and I have thought about using attr_reader + custom setter for attributes. However, the rationale for not doing so was that it might give the wrong impression of the code.

For example, when I see attr_reader, the first thought is that the attribute is read-only by outsiders. But it's actually not the case, and not until you find the corresponding setter in the code you'll realize the attribute is both readable and writable.

Is there other drawback, perhaps performance-related, besides slightly longer code?

Owner

zuhao commented Jul 8, 2013

Yes I am aware of attr_*, and I have thought about using attr_reader + custom setter for attributes. However, the rationale for not doing so was that it might give the wrong impression of the code.

For example, when I see attr_reader, the first thought is that the attribute is read-only by outsiders. But it's actually not the case, and not until you find the corresponding setter in the code you'll realize the attribute is both readable and writable.

Is there other drawback, perhaps performance-related, besides slightly longer code?

@zuhao

This comment has been minimized.

Show comment
Hide comment
@zuhao

zuhao Jul 8, 2013

Owner

Or maybe I can use attr_accessor and override the setter? This sounds like a better approach.

Owner

zuhao commented Jul 8, 2013

Or maybe I can use attr_accessor and override the setter? This sounds like a better approach.

@clbustos

This comment has been minimized.

Show comment
Hide comment
@clbustos

clbustos Jul 8, 2013

Zuhao:
Yes, I think it could be better, only if you delegate the responsability of
the validation in the object that you uses as an input.

On Sun, Jul 7, 2013 at 9:33 PM, Zuhao Wan notifications@github.com wrote:

Or maybe I can use attr_accessor and override the setter? This sounds
like a better approach.


Reply to this email directly or view it on GitHubhttps://github.com/zuhao/plotrb/issues/9#issuecomment-20581768
.

Claudio Bustos
Psicólogo
clbustos@gmail.com

clbustos commented Jul 8, 2013

Zuhao:
Yes, I think it could be better, only if you delegate the responsability of
the validation in the object that you uses as an input.

On Sun, Jul 7, 2013 at 9:33 PM, Zuhao Wan notifications@github.com wrote:

Or maybe I can use attr_accessor and override the setter? This sounds
like a better approach.


Reply to this email directly or view it on GitHubhttps://github.com/zuhao/plotrb/issues/9#issuecomment-20581768
.

Claudio Bustos
Psicólogo
clbustos@gmail.com

@mohawkjohn

This comment has been minimized.

Show comment
Hide comment
@mohawkjohn

mohawkjohn Jul 8, 2013

I agree with @clbustos. I think you should use attr_reader and write your own setter. That's a very common use-case. I believe attr_reader may in fact be faster.

mohawkjohn commented Jul 8, 2013

I agree with @clbustos. I think you should use attr_reader and write your own setter. That's a very common use-case. I believe attr_reader may in fact be faster.

@zuhao

This comment has been minimized.

Show comment
Hide comment
@zuhao

zuhao Jul 10, 2013

Owner

Thanks @clbustos and @mohawkjohn for reminding me. I apparently overlooked this issue in the first place. I've written a summary on this in my new blog post.

Owner

zuhao commented Jul 10, 2013

Thanks @clbustos and @mohawkjohn for reminding me. I apparently overlooked this issue in the first place. I've written a summary on this in my new blog post.

@zuhao zuhao closed this Jul 12, 2013

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