7
\$\begingroup\$

Originally, I had everything in the base method #read_wall(). I'm not sure what happened with the array, but when I tried:

array = result + second + third + fourth. I was left with data from only the original result. So I created this working disaster. Can you please help me refactor this?

   # Gets the user's Wall
      def read_wall(fbuserid)
         result ||= graph.get_connections(fbuserid, 'feed')
      end

  def second_wall(fbuserid)
     result ||= graph.get_connections(fbuserid, 'feed')
     second ||= result.next_page
  end

  def third_wall(fbuserid)
       result ||= graph.get_connections(fbuserid, 'feed')
       second ||= result.next_page
       third ||= second.next_page
  end

  def fourth_wall(fbuserid)
        result ||= graph.get_connections(fbuserid, 'feed')
        second ||= result.next_page
        third ||= second.next_page
        fourth ||= third.next_page
   end

  # Collects your friends' wall Posts and puts the IDs into an array
  def get_post_ids(fbuserid)
     x ||= read_wall(fbuserid)
     var = []
     for i in 0..25
        if find_nil(x, [i,'id']).nil? == false
           var << x[i]['id']
        end
     end

     second_wall ||= second_wall(fbuserid)
       for i in 0..25
          if find_nil(second_wall, [i,'id']).nil? == false
             var << second_wall[i]['id']
          end
       end

      third_wall ||= third_wall(fbuserid)
        for i in 0..25
           if find_nil(third_wall, [i,'id']).nil? == false
              var << third_wall[i]['id']
           end
        end

      fourth_wall ||= fourth_wall(fbuserid)
         for i in 0..25
            if find_nil(fourth_wall, [i,'id']).nil? == false
                 var << fourth_wall[i]['id']
            end
         end

      @get_post_ids = var
  end
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Is it meaningful in Ruby to declare a local variable with the same name as a function and still expect to reference both in the same context? second_wall ||= second_wall(fbuserid) ... find_nil(second_wall, ...) \$\endgroup\$ Feb 28, 2011 at 8:29
  • 1
    \$\begingroup\$ @David: Yes, x = x() works in ruby and sets the local variable x to the result of calling the method x. However it's not necessarily good style. \$\endgroup\$
    – sepp2k
    Feb 28, 2011 at 10:37
  • \$\begingroup\$ It should be obvious, but shadowing a method with a local variable is a terrible code smell, almost certain to cause confusion and trivial to avoid. So don't do it. There's no excuse. \$\endgroup\$ Apr 6, 2011 at 18:57

1 Answer 1

13
\$\begingroup\$
def fourth_wall(fbuserid)
  result ||= graph.get_connections(fbuserid, 'feed')
  second ||= result.next_page
  third ||= second.next_page
  fourth ||= third.next_page
end

A couple of problems here: result, second, third and fourth are all local variables. This means they won't persist after the method call is finished and they will never persist after the method call is finished. So:

  1. You don't need to set the fourth variable as you never use it in the method and it won't be available after
  2. You don't need ||= as the variables will never be set anyway.

I.e. your code is equivalent to this:

def fourth_wall(fbuserid)
  result = graph.get_connections(fbuserid, 'feed')
  second = result.next_page
  third = second.next_page
  third.next_page
end

Or just:

def fourth_wall(fbuserid)
  graph.get_connections(fbuserid, 'feed').next_page.next_page.next_page
end

The same goes for the other wall methods.


Further your fourth_wall method contains the entire code of the third_wall method (which contains the entire second_wall method and so on). This is bad style. Just let the methods call each other:

def fourth_wall(fbuserid)
  third_wall(fbuserid).next_page
end

(And again the same for the other wall methods of course)


However since you seem to only call those functions together (i.e. you never use fourth_wall without having used third_wall before) it seems much more sensible to have one method which returns all for walls:

def four_walls(fbuserid)
  first = graph.get_connections(fbuserid, 'feed')
  # For more than 4 pages the following should be replaced by a loop
  second = first.next_page
  third = second.next_page
  fourth = third.next_page
  [first, second, third, fourth]
end

In your get_post_ids method you're repeating the same code four times. This is again bad style. You're also creating an empty array and then appending to it from a loop. This is often (but not always) a sign of bad code in ruby as well. Also using x and var as variable names is not a good idea and having a variable name that starts with @get_ just seems weird and confusing.

I'm also assuming that your find_nil method is supposed to check whether either wall[i] or wall[i]['id'] are nil. There are better ways to do this.

In this case you can use map on the walls and then the numbers from 0 to 25 to create an array of user ids for each wall, then use compact to remove nils and flatten to turn the array into one single array rather than an array of four subarray. Thus your whole get_post_ids method can be replaced with this:

def retrieve_post_ids(fbuserid)
  walls = four_walls(fbuserid)
  @post_ids = walls.map do |wall|
    # Iterate over the first 26 entries in wall
    (0..25).map do |i|
      wall[i] && wall[i]['id']
     end.compact
  end.flatten
end

Though if the walls allow slicing similar to arrays, this would be better:

def retrieve_post_ids(fbuserid)
  walls = four_walls(fbuserid)
  @post_ids = walls.map do |wall|
    # Iterate over the first 26 entries in wall
    wall[0..25].map do |entry|
      entry && entry['id']
     end.compact
  end.flatten
end

If the wall never contains more than 26 elements, you don't even need the [0..25] and you can just use well.map.


However it occurs to me that the reason you needed the nil check at all was that each wall contains 25 elements and you got nil for index 25 because you did not realize that ranges using .. are inclusive. In that case you can just use:

def retrieve_post_ids(fbuserid)
  walls = four_walls(fbuserid)
  @post_ids = walls.map do |wall|
    # Iterate over the entries in wall
    wall.map do |entry|
      entry['id']
     end
  end.flatten
end
\$\endgroup\$
2
  • \$\begingroup\$ Everything I thought of with more besides! \$\endgroup\$ Mar 5, 2011 at 11:46
  • 1
    \$\begingroup\$ Maybe use flat_map instead of 1st map, so we'll not need flatten. \$\endgroup\$
    – Nakilon
    Apr 8, 2011 at 2:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.