Collecting friends' wall posts

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

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:

• You don't need to set the fourth variable as you never use it in the method and it won't be available after

• 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

StackExchange Code Review Q#1031, answer score: 13