From a3d5594248334502a6b204da8f9b51bc4bf5fa22 Mon Sep 17 00:00:00 2001 From: nicolasconnault Date: Wed, 30 May 2007 03:09:38 +0000 Subject: [PATCH] MDL-9506 Refactored grade_tree::get_tree by doing the following: - Extracted the $fillers array into a class variable - Extracted the addition of elements to grade_tree::fillers into a new add_fillers($array) method - Extracted the inclusion of fillers into the tree_array to a new include_fillers($tree, $object=NULL) method - Removed the building of the tree_filled, which was already performed by build_tree_filled() - Removed the generation of next_sortorder and previous_sortorder attributes, which was already performed by renumber() Result is a much leaner and scalable set of methods, and a tighter definition of responsibilities for the varying classes. I think I have also noticed a significant inprovement in speed. --- lib/grade/grade_category.php | 3 +- lib/grade/grade_tree.php | 268 ++++++++---------- .../grade/simpletest/testgradetree.php | 66 ++++- 3 files changed, 174 insertions(+), 163 deletions(-) diff --git a/lib/grade/grade_category.php b/lib/grade/grade_category.php index dce60e2d44b..585b2ffea67 100644 --- a/lib/grade/grade_category.php +++ b/lib/grade/grade_category.php @@ -321,7 +321,7 @@ class grade_category extends grade_object { $wheresql .= "iteminstance = $categoryid OR "; } $wheresql = substr($wheresql, 0, strrpos($wheresql, 'OR')); - $grade_items = set_field_select('grade_items', 'needsupdate', '1', $wheresql); + $grade_items = set_field_select('grade_items', 'needsupdate', '1', $wheresql . ' AND courseid = ' . $this->courseid); $this->grade_item->update_from_db(); } return $result; @@ -689,6 +689,7 @@ class grade_category extends grade_object { // If the associated grade_item isn't yet created, do it now. But first try loading it, in case it exists in DB. if (empty($grade_item->id)) { $grade_item->iteminstance = $this->id; + $grade_item->courseid = $this->courseid; $grade_item->itemtype = 'category'; $grade_item->insert(); $grade_item->update_from_db(); diff --git a/lib/grade/grade_tree.php b/lib/grade/grade_tree.php index 8001e6e0d04..218cf21fbab 100644 --- a/lib/grade/grade_tree.php +++ b/lib/grade/grade_tree.php @@ -53,6 +53,12 @@ class grade_tree { * @var array $tree_filled */ var $tree_filled = array(); + + /** + * An array of grade_items and grade_categories that have no parent and are not top-categories. + * @var arra $fillers + */ + var $fillers = array(); /** * An array of objects that need updating (normally just grade_item.sortorder). @@ -399,13 +405,14 @@ class grade_tree { foreach ($elements as $key => $element) { $this->first_sortorder++; $new_sortorder = $this->first_sortorder; + + $element['object']->previous_sortorder = $this->get_neighbour_sortorder($element, 'previous'); + $element['object']->next_sortorder = $this->get_neighbour_sortorder($element, 'next'); if (!empty($element['children'])) { $newtree[$this->first_sortorder] = $element; $newtree[$this->first_sortorder]['children'] = $this->renumber($this->first_sortorder, $element['children']); } else { - $element['object']->previous_sortorder = $this->get_neighbour_sortorder($element, 'previous'); - $element['object']->next_sortorder = $this->get_neighbour_sortorder($element, 'next'); $newtree[$this->first_sortorder] = $element; } @@ -478,8 +485,8 @@ class grade_tree { $returnnextelement = false; $count = 0; - foreach ($array as $sortorder => $child) { - + foreach ($array as $key => $child) { + $sortorder = $child['object']->sortorder; if ($returnnextelement) { return $sortorder; } @@ -509,18 +516,106 @@ class grade_tree { return $result; } + /** + * Provided $this->fillers is ready, and given a $tree array and a grade_category or grade_item, + * checks the fillers array to see if the current element needs to be included before the given + * object, and includes it if needed, or appends the filler to the tree if no object is given. + * The inserted filler is then deleted from the fillers array. The tree array is then returned. + * @param array $tree + * @param object $object Optional object before which to insert any fillers with a lower sortorder. + * If null, the current filler is appended to the tree. + * @return array $tree + */ + function include_fillers($tree, $object=NULL) { + if (empty($this->fillers)) { + return null; + } + + // Look at the current key of the fillers array. It is a sortorder. + if (key($this->fillers) < $object->sortorder || empty($object)) { + $sortorder = key($this->fillers); + $filler_object = current($this->fillers); + + // Remove filler so it doesn't get included again later + unset($this->fillers[$sortorder]); + + $element = array(); + + if (get_class($filler_object) == 'grade_category') { + $children = $filler_object->get_children(1); + unset($filler_object->children); + + $itemtree = array(); + + foreach ($children as $element) { + $finals = array(); + + if ($this->include_grades) { + $final = new grade_grades_final(); + $final->itemid = $element['object']->id; + $finals = $final->fetch_all_using_this(); + } + + $itemtree[$element['object']->sortorder] = array('object' => $element['object'], 'finalgrades' => $finals); + } + + $element['children'] = $itemtree; + } elseif (get_class($filler_object) == 'grade_item' && $this->include_grades) { + $final_grades = $filler_object->get_final(); + unset($filler_object->grade_grades_final); + $element['final_grades'] = $final_grades; + } + + $filler_object->sortorder = $sortorder; + + $element['object'] = $filler_object; + $tree[$sortorder] = $element; + } + + return $tree; + } + + /** + * Given an array of grade_categories or a grade_items, guesses whether each needs to be added to the fillers + * array or not (by checking children if a category, or checking parents if an item). It then + * instantiates the objects if needed and adds them to the fillers array. The element is then + * removed from the given array of objects, and the array is returned. + * @param array $object array of stdClass objects or grade_categories or grade_items + */ + function add_fillers($objects) { + foreach ($objects as $key => $object) { + + if (get_class($object) == 'grade_item' || !empty($object->itemname)) { + + if (empty($object->categoryid)) { + $item = new grade_item($object); + $sortorder = $item->get_sortorder(); + if (!empty($sortorder)) { + $this->fillers[$sortorder] = $item; + } + } + + } elseif (get_class($object) == 'grade_category' || !empty($object->fullname)) { + $topcatobject = new grade_category($object, false); + + if ($topcatobject->get_childrentype() == 'grade_item' && empty($topcatobject->parent)) { + $topcatobject->childrencount = $topcatobject->has_children(); + $this->fillers[$object->sortorder] = $topcatobject; + unset($objects[$key]); + } + } + } + + return $objects; + } + /** * Static method that returns a sorted, nested array of all grade_categories and grade_items for * a given course, or for the entire site if no courseid is given. - * @TODO Break this up in more nuclear methods - * @TODO Apply recursion to tree-building code (get_tree($first_parent=NULL)) - * NOTE the above todos are tricky in this instance because we are building two arrays simultaneously: tree_array and tree_filled * @return array */ function get_tree() { global $CFG; - global $db; - $db->debug = false; $tree = array(); $fillers = array(); @@ -538,22 +633,13 @@ class grade_tree { // Get ordered list of grade_items (not category type) $query = "SELECT * FROM $items_table WHERE itemtype <> 'category' $itemconstraint ORDER BY sortorder"; $grade_items = get_records_sql($query); - + if (empty($grade_items)) { return null; } // For every grade_item that doesn't have a parent category, create category fillers - foreach ($grade_items as $itemid => $item) { - if (empty($item->categoryid)) { - $item = new grade_item($item); - if (empty($item->sortorder)) { - $fillers[] = $item; - } else { - $fillers[$item->sortorder] = $item; - } - } - } + $grade_items = $this->add_fillers($grade_items); // Get all top categories $query = "SELECT $category_table.*, sortorder FROM $category_table, $items_table @@ -569,75 +655,15 @@ class grade_tree { } // If any of these categories has grade_items as children, create a topcategory filler with colspan=count(children) - foreach ($topcats as $topcatid => $topcat) { - $topcatobject = new grade_category($topcat, false); - if ($topcatobject->get_childrentype() == 'grade_item' && empty($topcatobject->parent)) { - $topcatobject->childrencount = $topcatobject->has_children(); - $fillers[$topcat->sortorder] = $topcatobject; - unset($topcats[$topcatid]); - } - } - - $last_topsortorder = null; + $topcats = $this->add_fillers($topcats); foreach ($topcats as $topcatid => $topcat) { - $last_subsortorder = null; // Check the fillers array, see if one must be inserted before this topcat - if (key($fillers) < $topcat->sortorder) { - $sortorder = key($fillers); - $object = current($fillers); - unset($fillers[$sortorder]); - - $this->tree_filled[$sortorder] = $this->get_filler($object); - $element = array(); - - if (get_class($object) == 'grade_category') { - $children = $object->get_children(1); - unset($object->children); - $last_itemsortorder = null; - $itemtree = array(); - - foreach ($children as $element) { - $finals = array(); - - if ($this->include_grades) { - $final = new grade_grades_final(); - $final->itemid = $element['object']->id; - $finals = $final->fetch_all_using_this(); - } - - $element['object']->previous_sortorder = $last_itemsortorder; - $itemtree[$element['object']->sortorder] = array('object' => $element['object'], 'finalgrades' => $finals); - - if (!empty($itemtree[$last_itemsortorder])) { - $itemtree[$last_itemsortorder]['object']->next_sortorder = $element['object']->sortorder; - } - - $last_itemsortorder = $element['object']->sortorder; - } - - $element['children'] = $itemtree; - } elseif (get_class($object) == 'grade_item' && $this->include_grades) { - $final_grades = $object->get_final(); - unset($object->grade_grades_final); - $element['final_grades'] = $final_grades; - } - - $object->sortorder = $sortorder; - $object->previous_sortorder = $last_topsortorder; - $element['object'] = $object; - $tree[$sortorder] = $element; - - if (!empty($tree[$last_topsortorder])) { - $tree[$last_topsortorder]['object']->next_sortorder = $sortorder; - } - - $last_topsortorder = $sortorder; - } + $tree = $this->include_fillers($tree, $topcat); $query = "SELECT $category_table.*, sortorder FROM $category_table, $items_table - WHERE iteminstance = $category_table.id AND parent = $topcatid ORDER BY sortorder"; + WHERE iteminstance = $category_table.id AND parent = $topcatid $catconstraint ORDER BY sortorder"; $subcats = get_records_sql($query); $subcattree = array(); @@ -648,7 +674,6 @@ class grade_tree { foreach ($subcats as $subcatid => $subcat) { $itemtree = array(); $items = get_records('grade_items', 'categoryid', $subcatid, 'sortorder'); - $last_itemsortorder = null; if (empty($items)) { continue; @@ -667,91 +692,26 @@ class grade_tree { $item = new grade_item($item); $item->sortorder = $sortorder; - $item->previous_sortorder = $last_itemsortorder; $itemtree[$item->sortorder] = array('object' => $item, 'finalgrades' => $finals); - - if (!empty($itemtree[$last_itemsortorder])) { - $itemtree[$last_itemsortorder]['object']->next_sortorder = $item->sortorder; - } - - $last_itemsortorder = $item->sortorder; } $sortorder = $subcat->sortorder; $subcat = new grade_category($subcat, false); $subcat->sortorder = $sortorder; - $subcat->previous_sortorder = $last_subsortorder; - $subcattree[$subcat->sortorder] = array('object' => $subcat, 'children' => $itemtree); - - if (!empty($subcattree[$last_subsortorder])) { - $subcattree[$last_subsortorder]['object']->next_sortorder = $subcat->sortorder; - } - - $last_subsortorder = $subcat->sortorder; + $subcattree[$subcat->sortorder] = array('object' => $subcat, 'children' => $itemtree); } $sortorder = $topcat->sortorder; $topcat = new grade_category($topcat, false); $topcat->sortorder = $sortorder; - $topcat->previous_sortorder = $last_topsortorder; - $tree[$topcat->sortorder] = array('object' => $topcat, 'children' => $subcattree); - $this->tree_filled[$topcat->sortorder] = array('object' => $topcat, 'children' => $subcattree); - - if (!empty($topcattree[$last_topsortorder])) { - $topcattree[$last_topsortorder]['object']->next_sortorder = $topcat->sortorder; - } - - $last_topsortorder = $topcat->sortorder; + $tree[$topcat->sortorder] = array('object' => $topcat, 'children' => $subcattree); } // If there are still grade_items or grade_categories without a top category, add another filler - if (!empty($fillers)) { - foreach ($fillers as $sortorder => $object) { - $this->tree_filled[$sortorder] = $this->get_filler($object); - - if (get_class($object) == 'grade_category') { - $children = $object->get_children(1); - unset($object->children); - $last_itemsortorder = null; - $itemtree = array(); - - foreach ($children as $element) { - $finals = array(); - - if ($this->include_grades) { - $final = new grade_grades_final(); - $final->itemid = $element['object']->id; - $finals = $final->fetch_all_using_this(); - } - - $element['object']->previous_sortorder = $last_itemsortorder; - $itemtree[$element['object']->sortorder] = array('object' => $element['object'], 'finalgrades' => $finals); - - if (!empty($itemtree[$last_itemsortorder])) { - $itemtree[$last_itemsortorder]['object']->next_sortorder = $element['object']->sortorder; - } - - $last_itemsortorder = $element['object']->sortorder; - } - - $element['children'] = $itemtree; - } elseif (get_class($object) == 'grade_item' && $this->include_grades) { - $final_grades = $object->get_final(); - unset($object->grade_grades_final); - $element['final_grades'] = $final_grades; - } - - $object->sortorder = $sortorder; - $object->previous_sortorder = $last_topsortorder; - $element['object'] = $object; - $tree[$sortorder] = $element; - - if (!empty($tree[$last_topsortorder])) { - $tree[$last_topsortorder]['object']->next_sortorder = $sortorder; - } - - $last_topsortorder = $sortorder; + if (!empty($this->fillers)) { + foreach ($this->fillers as $sortorder => $object) { + $tree = $this->include_fillers($tree); } } diff --git a/lib/simpletest/grade/simpletest/testgradetree.php b/lib/simpletest/grade/simpletest/testgradetree.php index 790c72ecd28..55bc4337c07 100644 --- a/lib/simpletest/grade/simpletest/testgradetree.php +++ b/lib/simpletest/grade/simpletest/testgradetree.php @@ -37,7 +37,8 @@ require_once($CFG->libdir . '/simpletest/testgradelib.php'); class grade_tree_test extends gradelib_test { function test_grade_tree_get_neighbour_sortorder() { $tree = new grade_tree($this->courseid); - + $tree->renumber(); + $element = $tree->locate_element(4); $this->assertEqual(3, $tree->get_neighbour_sortorder($element, 'previous')); $this->assertNull($tree->get_neighbour_sortorder($element, 'next')); @@ -60,11 +61,13 @@ class grade_tree_test extends gradelib_test { } + // TODO write more thorough and useful tests here. The renumber method assigns previous_sortorder and next_sortorder variables function test_grade_tree_renumber() { $tree = new grade_tree($this->courseid); - $tree1 = $tree; + $this->assertTrue(empty($tree->tree_array[1]['object']->next_sortorder)); $tree->renumber(); - $this->assertEqual($tree1->tree_array[1]['object'], $tree->tree_array[1]['object']); + $this->assertFalse(empty($tree->tree_array[1]['object']->next_sortorder)); + $this->assertTrue(empty($tree->need_update)); } @@ -123,8 +126,21 @@ class grade_tree_test extends gradelib_test { } function test_grade_tree_move_element() { + /* 0. + * Starting layout: + *__________________ + *|_________1_______| ____________ + *|__2__|_____4_____|_____|_____8____| + *|__3__|__5__|__6__|__7__|__9__|_10_| + */ $tree = new grade_tree($this->courseid); - + /* 1. + * Desired result: + *_____________ + *|_____1_____| _________________ + *|__2__|__4__|_____|________7_______| + *|__3__|__5__|__6__|__8__|__9__|_10_| + */ $tree->move_element(4, 10); $this->assertFalse(empty($tree->tree_array[8]['children'][1])); $this->assertEqual('unittestgradeitem2', $tree->tree_array[8]['children'][1]['object']->itemname); @@ -142,23 +158,48 @@ class grade_tree_test extends gradelib_test { $this->assertFalse(empty($tree->tree_array[1]['children'][4]['children'][5])); $this->assertEqual('unittestgradeitem3', $tree->tree_array[1]['children'][4]['children'][5]['object']->itemname); - + $tree->need_update = array(); + + /* 2. + * Desired result: + *___________________ + *|________1________|_________________ + *|_____2_____|__5__|________7_______| + *|__3__|__4__|__6__|__8__|__9__|_10_| + */ $tree->move_element(6, 3, 'after'); $this->assertFalse(empty($tree->tree_array[1]['children'][2]['children'][1])); $this->assertEqual('unittestorphangradeitem1', $tree->tree_array[1]['children'][2]['children'][1]['object']->itemname); $tree->renumber(); + $this->assertEqual(4, count($tree->need_update)); $this->assertFalse(empty($tree->tree_array[1]['children'][2]['children'][4])); $this->assertEqual('unittestorphangradeitem1', $tree->tree_array[1]['children'][2]['children'][4]['object']->itemname); + $tree->need_update = array(); // Try moving a subcategory + /* 3. + * Desired result: + *___________________ + *|________1________|_________________ + *|__2__|_____4_____|________7_______| + *|__3__|__5__|__6__|__8__|__9__|_10_| + */ $tree->move_element(2, 5, 'after'); $this->assertFalse(empty($tree->tree_array[1]['children'][1])); $this->assertEqual('unittestcategory2', $tree->tree_array[1]['children'][1]['object']->fullname); $tree->renumber(); + $this->assertEqual(5, count($tree->need_update)); $this->assertFalse(empty($tree->tree_array[1]['children'][4])); $this->assertEqual('unittestcategory2', $tree->tree_array[1]['children'][4]['object']->fullname); + $tree->need_update = array(); - // Try moving a subcategory + /* 4. + * Desired result: + *_________________________ + *|___________1___________|____________ + *|__2__|________4________|_____8_____| + *|__3__|__5__|__6__|__7__|__9__|_10__| + */ $tree = new grade_tree($this->courseid); $original_count = count($tree->tree_array, COUNT_RECURSIVE); $tree->move_element(8, 5); @@ -167,16 +208,26 @@ class grade_tree_test extends gradelib_test { $this->assertFalse(empty($tree->tree_array[1]['children'][1])); $this->assertEqual('level1category', $tree->tree_array[1]['children'][1]['object']->fullname); $tree->renumber(); + $this->assertEqual(4, count($tree->need_update)); $this->assertFalse(empty($tree->tree_array[1]['children'][5])); $this->assertEqual('level1category', $tree->tree_array[1]['children'][5]['object']->fullname); $this->assertEqual('singleparentitem1', $tree->tree_array[1]['children'][5]['children'][6]['object']->itemname); + $tree->need_update = array(); // Try moving a top category + /* 5. + * Desired result: + * ___________________ + * |_________2_______|___________ + *______|__3__|_____5_____|_____8____| + *|__1__|__4__|__6__|__7__|__9__|_10_| + */ $tree = new grade_tree($this->courseid); $tree->move_element(1, 8); $this->assertFalse(empty($tree->tree_array[1])); $this->assertEqual('unittestcategory1', $tree->tree_array[1]['object']->fullname); $tree->renumber(); + $this->assertEqual(7, count($tree->need_update)); $this->assertFalse(empty($tree->tree_array[2])); $this->assertEqual('unittestcategory1', $tree->tree_array[2]['object']->fullname); } @@ -188,6 +239,7 @@ class grade_tree_test extends gradelib_test { function test_grade_tree_display_grades() { $tree = new grade_tree($this->courseid); + $tree->build_tree_filled(); $result_html = $tree->display_grades(); $expected_html = '
unittestcategory1  
unittestcategory2unittestcategory3 level1category
unittestgradeitem1unittestgradeitem2unittestgradeitem3unittestorphangradeitem1singleparentitem1singleparentitem2
'; @@ -196,7 +248,6 @@ class grade_tree_test extends gradelib_test { function test_grade_tree_get_tree() { $tree = new grade_tree($this->courseid, true); - $this->assertEqual(58, count($tree->tree_filled, COUNT_RECURSIVE)); $this->assertEqual(48, count($tree->tree_array, COUNT_RECURSIVE)); } @@ -291,7 +342,6 @@ class grade_tree_test extends gradelib_test { function test_grade_tree_display_edit_tree() { $tree = new grade_tree($this->courseid); - echo $tree->get_edit_tree(); } }