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-lo...

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-lo... 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 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-lo... > > 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

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".

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(a)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

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.

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

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