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.

diff --git a/lib/grade/grade_category.php b/lib/grade/grade_category.php
index dce60e2d44bfd01aab4f46ca9fa122454f619cb5..585b2ffea672a04738ace061b3afc47247598f19 100644 (file)
--- 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 8001e6e0d045681f5698fcb9feba486804b39684..218cf21fbab6d846d0379260fdcc67abf69c69a0 100644 (file)
--- 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 790c72ecd28c6c8d7e7c4242bb09b4ed9c982e27..55bc4337c0759562c21145dd9372f8e4ba7418bc 100644 (file)
--- 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 = '<table style="text-align: center" border="1"><tr><th colspan="3">unittestcategory1</th><td class="topfiller">&nbsp;</td><td colspan="2" class="topfiller">&nbsp;</td></tr><tr><td colspan="2">unittestcategory2</td><td colspan="1">unittestcategory3</td><td class="subfiller">&nbsp;</td><td colspan="2">level1category</td></tr><tr><td>unittestgradeitem1</td><td>unittestgradeitem2</td><td>unittestgradeitem3</td><td>unittestorphangradeitem1</td><td>singleparentitem1</td><td>singleparentitem2</td></tr></table>';
@@ -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(); 
     }
     
 }