Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
Can someone with a more concrete understanding of the right way update the wiki page, or send me some pointers so I can do so? I'd love for the page to show the right way, maybe an example of rewriting the "bad way" into the right way, rather than just giving a stern warning that it's bad.
-- Matt
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
On 08/16/2012 11:05 AM, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
Can someone with a more concrete understanding of the right way update the wiki page, or send me some pointers so I can do so? I'd love for the page to show the right way, maybe an example of rewriting the "bad way" into the right way, rather than just giving a stern warning that it's bad.
-- Matt
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
In general while I don't completely avoid them, I tend to shy away from instance variables when I can as they can be used in a somewhat 'global' manner in rails views.
This doesn't usually pose a problem for primary data being operated on, for example a provider account which we ensure the user has privileges on, but for ancillary data, eg data that is loaded to be used in subsequent lookups or such, the scope of the variable assignment should be limited to where it is used. Combined w/ places which the views rendered were directly based on user params without any validation (almost of all these have been take care of by now), this could have potentially lead to data being presented to the user which they were not supposed to have access to.
The guideline might be a bit strongly worded, though it doesn't say instance variables are prohibited, just should be used only when necessary (as they often are). This mostly amounts to a scoping issue, don't use global variables and limit the exposure of data to just where it is used. We can leverage the named_scopes feature in the activerecord namespace going forward to filter what is made available, but for the time being following the guidelines should suffice
-Mo
On 08/16/2012 05:05 PM, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
Can someone with a more concrete understanding of the right way update the wiki page, or send me some pointers so I can do so? I'd love for the page to show the right way, maybe an example of rewriting the "bad way" into the right way, rather than just giving a stern warning that it's bad.
-- Matt
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you. I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post} end
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Jirka
On 17.8.2012 10:52, Jirka Tomasek wrote:
On 08/16/2012 05:05 PM, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
Can someone with a more concrete understanding of the right way update the wiki page, or send me some pointers so I can do so? I'd love for the page to show the right way, maybe an example of rewriting the "bad way" into the right way, rather than just giving a stern warning that it's bad.
-- Matt
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you. I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post} end
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Jirka
I've seen Petr use this approach in his code [1]. I like it because it makes very clear what variables the view needs to be able to render itself.
Jirka no. 2
[1] https://github.com/aeolusproject/conductor/blob/e8ea044f9c148b92d4038544fb48...
On Friday, August 17, 2012 10:52:46 AM Jirka Tomasek wrote:
On 08/16/2012 05:05 PM, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
Can someone with a more concrete understanding of the right way update the wiki page, or send me some pointers so I can do so? I'd love for the page to show the right way, maybe an example of rewriting the "bad way" into the right way, rather than just giving a stern warning that it's bad.
-- Matt
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you. I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post} end
Yes, this is the way I use them. One reason is to be clear what variables a view needs. Another one is that in this way I use no instance variables in views.
And why I am trying to avoid them in views? Because of implicit nil value when instance variable is not defined before. That brings problems when you rely on value of that variable. Something changes in controller or model and suddenly your view do not work and you have to find out why. I prefer explicit code when variable can be nil or undefined - we can use local_assigns[:variable_name] in view.
Partials thing Jirka mentioned - in past I often refactored views to partials and sometimes I used partials common for more resources. In that cases I hated how views dictated controllers what instance variables they had to define. I like to see when views are more detached from controllers.
We have to use instance variables when we use filters in controllers to manipulate some data in memory. Even in this case I would like to see solution not using before filters at all (for memory data manipulation) and I would use simple method call with return value.
But that is not the "Rails way" and in fact it is part of another discussion.
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Jirka
On 08/17/2012 01:22 PM, Petr Blaho wrote:
On Friday, August 17, 2012 10:52:46 AM Jirka Tomasek wrote:
On 08/16/2012 05:05 PM, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
Can someone with a more concrete understanding of the right way update the wiki page, or send me some pointers so I can do so? I'd love for the page to show the right way, maybe an example of rewriting the "bad way" into the right way, rather than just giving a stern warning that it's bad.
-- Matt
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you. I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post}
end
Yes, this is the way I use them. One reason is to be clear what variables a view needs. Another one is that in this way I use no instance variables in views.
And why I am trying to avoid them in views? Because of implicit nil value when instance variable is not defined before. That brings problems when you rely on value of that variable. Something changes in controller or model and suddenly your view do not work and you have to find out why. I prefer explicit code when variable can be nil or undefined - we can use local_assigns[:variable_name] in view.
Partials thing Jirka mentioned - in past I often refactored views to partials and sometimes I used partials common for more resources. In that cases I hated how views dictated controllers what instance variables they had to define. I like to see when views are more detached from controllers.
We have to use instance variables when we use filters in controllers to manipulate some data in memory. Even in this case I would like to see solution not using before filters at all (for memory data manipulation) and I would use simple method call with return value.
But that is not the "Rails way" and in fact it is part of another discussion.
These are quite reasonable arguments but personally I never felt it that problematic to avoid using them. Actually for me this is a neat feature of Ruby rather than an issue.
I think we should keep our coding standards in align with the standards of the Ruby and the Rails community. As Jirka wrote, there is no book on Rails that uses this approach, none of them matches with our coding guideline.
As Matt linked from Rails Best Practices, they only recommend it for partials, not for all views as we specify. Our strict approach may frighten off future contributors, so I vote for not creating stricter coding standards than reasonable and let developers choose which approach they prefer.
Imre
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Jirka
On Fri, Aug 17, 2012 at 02:12:28PM +0200, Imre Farkas wrote:
These are quite reasonable arguments but personally I never felt it that problematic to avoid using them. Actually for me this is a neat feature of Ruby rather than an issue.
I think we should keep our coding standards in align with the standards of the Ruby and the Rails community. As Jirka wrote, there is no book on Rails that uses this approach, none of them matches with our coding guideline.
This got me curious, so I just looked it up.
The rubyonrails.org guides recommend using instance variables for views: http://guides.rubyonrails.org/layouts_and_rendering.html
I'm not saying that we must take this as gospel, but I do think we need to have a pretty compelling reason to do something differently than most of the Rails community.
Of course, this is very different from blindly using instance variables where they're not needed -- "users.each {|@x| puts "#{@x.name}"}" would be absurd.
As Matt linked from Rails Best Practices, they only recommend it for partials, not for all views as we specify. Our strict approach may frighten off future contributors, so I vote for not creating stricter coding standards than reasonable and let developers choose which approach they prefer.
+1 to this
In general, I actually think we should all agree on a "right way" versus each using whichever approach we prefer. It seems like it would keep the code more consistent and easier to follow. But at the same time, I'm not sure that there's a huge amount to be gained by having an extended debate over variables right now.
-- Matt
On Fri, Aug 17, 2012 at 10:52:46AM +0200, Jirka Tomasek wrote:
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you.
Yes, that was the main source of my confusion.
I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
As someone mentioned in another reply here, there are uses where you really need them -- e.g., before_filters. Of course we should be careful to not use instance variables where there's no need for them; sometimes we'll mistakenly use an instance variable just within the scope of a method, for example.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post} end
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Yes, this feels pretty weird to me. That whole line should be unnecessary -- we're overriding the "Rails magic" to explicitly perform the default behavior, except referencing the variable as a local one. If anything, this seems _more_ error-prone to me than relying on instance variables.
-- Matt
On this note, I thought it would be good to raise having a "lint like" tool on the list;
I've been asking in IRC if there is a "lint like" tool available, this would make it easier for smaller contributors to at least avoid the majority of issues, providing they run it through the "lint like" tool, which has been configured per coding standards.
This is all conjecture of course untill such time as the tool is made available, whatever it may be.
Case and point, whilst I am very interested in "hacking" and contributing code, I am not in any way shape or form a ruby developer I am a systems admin, who just happens to be able to write some ruby; and prefers python (pylint for example is a great tool).
Thoughts?
Cheers
David (Oneiroi)
On Fri, Aug 17, 2012 at 2:39 PM, Matt Wagner matt.wagner@redhat.com wrote:
On Fri, Aug 17, 2012 at 10:52:46AM +0200, Jirka Tomasek wrote:
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you.
Yes, that was the main source of my confusion.
I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
As someone mentioned in another reply here, there are uses where you really need them -- e.g., before_filters. Of course we should be careful to not use instance variables where there's no need for them; sometimes we'll mistakenly use an instance variable just within the scope of a method, for example.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post} end
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Yes, this feels pretty weird to me. That whole line should be unnecessary -- we're overriding the "Rails magic" to explicitly perform the default behavior, except referencing the variable as a local one. If anything, this seems _more_ error-prone to me than relying on instance variables.
-- Matt
On 08/17/2012 03:42 PM, David Busby wrote:
On this note, I thought it would be good to raise having a "lint like" tool on the list;
I've been asking in IRC if there is a "lint like" tool available, this would make it easier for smaller contributors to at least avoid the majority of issues, providing they run it through the "lint like" tool, which has been configured per coding standards.
This is all conjecture of course untill such time as the tool is made available, whatever it may be.
Case and point, whilst I am very interested in "hacking" and contributing code, I am not in any way shape or form a ruby developer I am a systems admin, who just happens to be able to write some ruby; and prefers python (pylint for example is a great tool).
Thoughts?
Cheers
David (Oneiroi)
On Fri, Aug 17, 2012 at 2:39 PM, Matt Wagner <matt.wagner@redhat.com mailto:matt.wagner@redhat.com> wrote:
On Fri, Aug 17, 2012 at 10:52:46AM +0200, Jirka Tomasek wrote: > > I don't like the sentence "Don't use it for passing variables to > view. Never." which is actually one of the first things that any > Rails book teaches you. Yes, that was the main source of my confusion. > I agree that we should try to reduce using > instance variables in controllers where possible to avoid cluttering > controllers with them. As someone mentioned in another reply here, there are uses where you really need them -- e.g., before_filters. Of course we should be careful to not use instance variables where there's no need for them; sometimes we'll mistakenly use an instance variable just within the scope of a method, for example. > The blog post you sent points out the reusability of partials so > passing the variables as locals helps avoid requiring use of eg. > @post in all partials instances. Imho it depends on the desired use > of the partial. > > We have discussed with Imre the alternative way to pass variables > from controller's action to view. It can be done similar way as with > partials: > > ...someactioncode... > render :action => :index, :locals => {:post => post} > end > > > but I have never seen this used anywhere in controller and I assume > this is not the "Rails way". Yes, this feels pretty weird to me. That whole line should be unnecessary -- we're overriding the "Rails magic" to explicitly perform the default behavior, except referencing the variable as a local one. If anything, this seems _more_ error-prone to me than relying on instance variables. -- Matt
Lint like tool is called rails_best_practices, and is in process of packaging.
maros
On 08/17/2012 03:39 PM, Matt Wagner wrote:
On Fri, Aug 17, 2012 at 10:52:46AM +0200, Jirka Tomasek wrote:
I don't like the sentence "Don't use it for passing variables to view. Never." which is actually one of the first things that any Rails book teaches you.
Yes, that was the main source of my confusion.
I agree that we should try to reduce using instance variables in controllers where possible to avoid cluttering controllers with them.
As someone mentioned in another reply here, there are uses where you really need them -- e.g., before_filters. Of course we should be careful to not use instance variables where there's no need for them; sometimes we'll mistakenly use an instance variable just within the scope of a method, for example.
The blog post you sent points out the reusability of partials so passing the variables as locals helps avoid requiring use of eg. @post in all partials instances. Imho it depends on the desired use of the partial.
We have discussed with Imre the alternative way to pass variables from controller's action to view. It can be done similar way as with partials:
...someactioncode... render :action => :index, :locals => {:post => post} end
but I have never seen this used anywhere in controller and I assume this is not the "Rails way".
Yes, this feels pretty weird to me. That whole line should be unnecessary -- we're overriding the "Rails magic" to explicitly perform the default behavior, except referencing the variable as a local one. If anything, this seems _more_ error-prone to me than relying on instance variables.
All magic in programming is magic at wrong place. Relying on autovivified variables for passing values is 1) unsafe 2) unreadable 3) unmaintainable 4) spaghetti This can cause hell especially when refactoring so you lose track of which data comes from where. Then your variables became automagically initialised to nil, and then kaboom.
Generally, you want to have variables of only one type. Since types in ruby are not lifted with nil, then by autovivification you have broken contract of returning correct data type.
Nice example of this missbehavior might be returning nil instead of [] when method can't find any items in database (this is just an illustrative example). So you have to check first if there is a nil in variable and only then you can use it (for example call 'each' method on that).
When using locals, interpreter can much easier identify real source of problem, so you get error message stating that variable does not exist, instead of confusing error stating that you can't call something on nil, including much worse problem when that 'something' called on nil passes without problems but produces completely undesired result (i.e. hidden runtime bug & data corruption).
Convenience or (at least partial) correctness and safety?
PS: If this sounds rude, I apologise, it was not meant like that.
- maros
On Fri, Aug 17, 2012 at 04:05:24PM +0200, Maros Zatko wrote:
All magic in programming is magic at wrong place.
I was referring to mostly the Rails pattern of convention over configuration. If you don't explicitly call render in a controller method, Rails knows where to look. If you have a Cat model, Rails assumes it's backed by "cats" in the table. And if you define an instance variable in a controller, Rails makes it available to the views.
Relying on autovivified variables for passing values is
- unsafe
- unreadable
- unmaintainable
- spaghetti
This can cause hell especially when refactoring so you lose track of which data comes from where. Then your variables became automagically initialised to nil, and then kaboom.
But if you lose track of what you're doing during a refactor, isn't it _good_ if things blow up?
Generally, you want to have variables of only one type. Since types in ruby are not lifted with nil, then by autovivification you have broken contract of returning correct data type.
I'm not sure what you follow by only one type. Do you mean that having one variable that's an Array and another that's a Hash is bad?
Nice example of this missbehavior might be returning nil instead of [] when method can't find any items in database (this is just an illustrative example).
So you have to check first if there is a nil in variable and only then you can use it (for example call 'each' method on that).
Yes, this can be annoying.
When using locals, interpreter can much easier identify real source of problem, so you get error message stating that variable does not exist, instead of confusing error stating that you can't call something on nil, including much worse problem when that 'something' called on nil passes without problems but produces completely undesired result (i.e. hidden runtime bug & data corruption).
But with all due respect, "If you carelessly refactor code, the errors you cause might be confusing" doesn't strike me as a compelling argument to do something differently from most other projects and from the Rails documentation.
Convenience or (at least partial) correctness and safety?
I wouldn't use any of those terms to describe any of this, personally. I'd say it's more a question of whether doing things our own unique way that requires explicitly passing variables along into views is any safer than using the Rails way that makes them automatically available.
PS: If this sounds rude, I apologise, it was not meant like that.
Likewise -- I hope our difference of opinions on this doesn't seem rude.
-- Matt
On Thu, 2012-08-16 at 11:05 -0400, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
I've seen that statement in the coding guidelines, and have simply been mystified by it. It seems to take a reasonable rule (reusable partials shouldn't rely on instance variables) to an absurd extreme (never use instance variables to pass data to a view)
The dividing line should be whether a particular view/partial is reusable, or meant to be reusable. As long as a view is only used from one place, I don't see anything wrong with using instance variables. Note that that doesn't mean partials should never use instance variables - there's more reasons than reuse to split part of a view into a partial (AJAX updates, for example)
David
[1] https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
[2] http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local...
On 08/17/2012 05:12 PM, David Lutterkort wrote:
On Thu, 2012-08-16 at 11:05 -0400, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
I've seen that statement in the coding guidelines, and have simply been mystified by it. It seems to take a reasonable rule (reusable partials shouldn't rely on instance variables) to an absurd extreme (never use instance variables to pass data to a view)
The dividing line should be whether a particular view/partial is reusable, or meant to be reusable. As long as a view is only used from one place, I don't see anything wrong with using instance variables. Note that that doesn't mean partials should never use instance variables
- there's more reasons than reuse to split part of a view into a partial
(AJAX updates, for example)
David
+1. Would be in favour of rewording the code guidelines to take this approach.
-Mo
On 08/21/2012 07:36 AM, Mo Morsi wrote:
On 08/17/2012 05:12 PM, David Lutterkort wrote:
On Thu, 2012-08-16 at 11:05 -0400, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
I've seen that statement in the coding guidelines, and have simply been mystified by it. It seems to take a reasonable rule (reusable partials shouldn't rely on instance variables) to an absurd extreme (never use instance variables to pass data to a view)
The dividing line should be whether a particular view/partial is reusable, or meant to be reusable. As long as a view is only used from one place, I don't see anything wrong with using instance variables. Note that that doesn't mean partials should never use instance variables
- there's more reasons than reuse to split part of a view into a partial
(AJAX updates, for example)
David
+1. Would be in favour of rewording the code guidelines to take this approach.
-Mo
+1
Jirka
On 08/21/2012 03:32 PM, Jirka Tomasek wrote:
On 08/21/2012 07:36 AM, Mo Morsi wrote:
On 08/17/2012 05:12 PM, David Lutterkort wrote:
On Thu, 2012-08-16 at 11:05 -0400, Matt Wagner wrote:
Hi folks,
I've noticed that we've been objecting to patches using instance variables to pass information to views, so I went to see our coding guidelines page[1] to read about why.
It makes very clear that it's bad, but it doesn't really explain the right way. I'd hoped that the Coding Guidelines page would be more of a guide on how to do things the right way.
I've really only found one blog post[2] that addresses the issue, and it doesn't really give a great example. It still uses an instance variable in the controller, but inside of a view it passes it as a local variable. It's more specific, sure, but it's not abundantly clear how that actually makes anything better, or why an instance variable from the controller to the view is okay but a view to a partial is not okay.
I've seen that statement in the coding guidelines, and have simply been mystified by it. It seems to take a reasonable rule (reusable partials shouldn't rely on instance variables) to an absurd extreme (never use instance variables to pass data to a view)
The dividing line should be whether a particular view/partial is reusable, or meant to be reusable. As long as a view is only used from one place, I don't see anything wrong with using instance variables. Note that that doesn't mean partials should never use instance variables
- there's more reasons than reuse to split part of a view into a partial
(AJAX updates, for example)
David
+1. Would be in favour of rewording the code guidelines to take this approach.
-Mo
+1
Jirka
Updated the wiki page, you can check it out: https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Coding_Guidelines
Imre
aeolus-devel@lists.fedorahosted.org