Can you help me to optimize my code for the following problem:

An array of hashes named expected_tasks has tasks. Each task has a user that has to execute an action and contains fields. The expected_tasks are related to the actual_tasks. We want to execute the expected_task[action] on the actual_task["id]". Therefor want to be able to select the actual_task["id"] where all the fields have the same value.

In the end this means that the expected_tasks[0] matches to the actual_tasks[0] because their fields are exactly the same. When their is a match, we can use actual_task["id"].

This is what the data looks like:

#the fields_to_check is an extraction of the keys of the expected_tasks minus user and action
fields_to_check = [ 'field_to_check_1', 'field_to_check_2' ] 

expected_tasks = [{ 'user' => 'user', 
                     'action' => 'action_y', 
                     'field_to_check_1' => 'value_a', 
                     'field_to_check_2' => 'value_a' },{
                     'user' => 'user', 
                     'action' => 'action_x', 
                     'field_to_check_1' => 'value_b', 
                     'field_to_check_2' => 'value_b'} ]

sparse_actual_subtasks = [{ 'id' => '1' , 
                     'field_to_check_1' => 'value_b', 
                     'field_to_check_2' => 'value_b' },{
                     'id' => '2' , 
                     'field_to_check_1' => 'value_a', 
                     'field_to_check_2' => 'value_a' }]

The piece of code to map the id from actual_task to the right action + user from the expected_task:

expected_tasks.each do |expected_task|

  mapped_tasks = []

  actual_task = sparse_actual_subtasks.find do |actual_task|
    fields_to_check.all? do |field| 
    expected_task[field] == actual_task[field] 

  #map action and user to task with id 
  actual_task['action'] = expected_task['action']
  actual_task['user'] = expected_task['user']
  mapped_tasks << actual_task

  puts mapped_tasks

  • \$\begingroup\$ I could not understand your explanation above :'( \$\endgroup\$
    – BenKoshy
    Oct 12, 2016 at 13:31
  • \$\begingroup\$ I am going to try and rewrite :D @BKSpurgeon \$\endgroup\$
    – Woots
    Oct 12, 2016 at 13:38
  • \$\begingroup\$ In this rewrite please paste data structure examples that people can paste on their code. \$\endgroup\$
    – tokland
    Oct 13, 2016 at 7:30
  • \$\begingroup\$ @tokland I have fixed the json structure and tested the code. It is working as expected now. \$\endgroup\$
    – Woots
    Oct 13, 2016 at 8:41

1 Answer 1

def mapped_tasks(expected, subtasks, fields)
  fields_match = ->(t1, t2) {fields.all? {|f| t1[f] == t2[f]}}
  expected.map do |e| 
    subtask = subtasks.find {|s| fields_match.(e,s)}
    subtask ? e.merge(subtask) : nil

should do the same as your original.

  • use merge rather than explicitly adding each field you want
  • create a lambda to name the condition of "all relevant fields matching". this makes the loop within which it is called more readable.
  • rather than procedurally declaring an array and adding items to it, simply use map to chang all the expected items. the ones that don't match are changed to nil, and we remove them at the end using compact

notes on the original code:

  • the data structures that we're assuming exist seem ill-fitted to the problem. if they're within your control, consider simplifying them. this should make the solution to the problem here more natural
  • avoid extraneous information in your variable names. it gives the illusion of greater clarity but in fact makes the code harder to read at a glance. eg fields is a clearer name than fields_to_check

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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