# 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
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)
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

• 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, ...) Feb 28, 2011 at 8:29
• @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. Feb 28, 2011 at 10:37
• 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. Apr 6, 2011 at 18:57

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

• Everything I thought of with more besides! Mar 5, 2011 at 11:46
• Maybe use flat_map instead of 1st map, so we'll not need flatten. Apr 8, 2011 at 2:29