Jump to content

The ultimate community for Ruby on Rails developers.


Photo

Simple refactor?


  • Please log in to reply
3 replies to this topic

#1 dcastellano1

dcastellano1

    Signalman

  • Members
  • 13 posts
  • LocationColumbus, Ohio

Posted 14 December 2013 - 06:49 PM

Hi,

 

I am wondering if there is a more condensed way to write the following (trying to improve my skills and do some refactoring)...

 

    if [ session[:image_minisection_id],
        session[:image_subsection_id],
        session[:image_section_id],
        session[:image_chapter_id],
        session[:image_book_id],
        session[:image_subject_id]].all? { |x| x.blank? }
then
       session[:image_minisection_id]   = session[:minisection_id]
       session[:image_subsection_id]    = session[:subsection_id]
       session[:image_section_id]          = session[:section_id]
       session[:image_chapter_id]          = session[:chapter_id]
       session[:image_book_id]              = session[:book_id]
       session[:image_subject_id]           = session[:subject_id]

 

Have tried multiple variations of the following:

 

then
      [ minisection, subsection, section, chapter, book, subject].each do |y|
        "#{'session[:image_' + y + '_id]'}" = "#{'session[:' + y + '_id]'}"  
 
 but can't seem to get it to work???
 
Any help would be appreciated.
 
Dave


#2 james

james

    Guard

  • Moderators
  • 221 posts
  • LocationLeeds, U.K.

Posted 14 December 2013 - 08:25 PM

That if statement feels totally weird, it's a little difficult to understand the intention but what I think you are looking for is to drop the if altogether and use or equal instead

session[:image_minisection_id] ||= session[:minisection_id]

This means

if session[:image_minisection_id] has a value then leave it alone otherwise set it to session[:minisection_id]

 

But that is slightly different to what you currently have. What I think your current condition sais is

 

if there is a key called image_minisection_id in the session hash then regardless of the value contained in that key set the value to equal the value stored in minisection_id

 

Another observation, you really shouldn't be relying on the session so heavily.

 

Perhaps a clearer answer might be available if you could ask the question in English rather than in code to enabe a better understanding of the intention rather than what you actually have


Programming is just about problem solving!


#3 dcastellano1

dcastellano1

    Signalman

  • Members
  • 13 posts
  • LocationColumbus, Ohio

Posted 14 December 2013 - 09:28 PM

James,

 

 

 

I am using the if statement to set the initial selections in a group of dropdown menus.  As the user navigates to the page where the dropdowns reside (image gallery page, they either drill down through an catalog of books, chapters, sections, ect setting the corresponding sessions eg session[:book] as they go.  In this case when they arrive at the gallery, the session[:x_id}'s are present but the session[:image_x_id]'s are empty.  This is where the condition you looked at is met and the session[:image_x_id]'s need to be set to the session[:x_id}'s.  The reason for this is once the user changes the dropdown selections, the session[:x_id}'s need to be "remembered" , so when an image is saved  it can be categorized based on the saved session[:x_id}'s.   The session[:image_x_id]'s are set as the user navigates thru the image gallery itself to select the images to show based on the dropdowns selected.  I could not think of a good alternative to using the sessions for this.

 

 

The condition is only true if all 6 session[:image_x_id]'s are empty,  and then they need to be set equal to their matching session[:x_id}'s.  The case of them all being empty can only occur in one situation.  If only some of the session[:image_x_id]'s are empty, the empty session[:image_x_id]'s do not need to be set equal to their matching session[:x_id}'s and need to be left empty.

 

I agree with it being a weird feeling statement.  I have lots of them and now that I have stuff working am trying to clean everything up a bit.

 

 

 

 

 

Dave

 

btw,  I live in Columbus, Ohio but went to school at Ackworth  in the 70's and lived in Leeds which is where by family is from.  Is Roundhay park still there?



#4 james

james

    Guard

  • Moderators
  • 221 posts
  • LocationLeeds, U.K.

Posted 14 December 2013 - 10:05 PM

Roundhay Park is indeed still there and as beautiful as ever, In fact I spend quite a lot of time there, it's just down the road :)
 

 

the session[:x_id}'s are present but the session[:image_x_id]'s are empty.  This is where the condition you looked at is met and the session[:image_x_id]'s need to be set to the session[:x_id}'s.

 

 

So for this particular condition you are asking if all the session[:image_...] have no value?

 

That would normally be written using &&'s but there isn't really a shortcut to checking them all and in this case I think it is a case of if it works, don't fix it. I really don't think that the shortcut you are looking for exists in this case.

 

Because you are stuffing them all into an array to do the check it might be slightly more performant to change to using &&'s but that would be slightly more long winded coding wise.


Programming is just about problem solving!





0 user(s) are reading this topic

0 members, 0 guests, 0 anonymous users