One example is the calendar_plugin_style::calendar_build_month()

This one function is 300 lines of code!

If the tasks involved with the function were broken into smaller functions that could be overridden, then extending this plugin would be a breeze... However, if you want to extend the month output and alter it just a little bit, you have to re-write 300 lines of code. It would be great if some of these functions were smaller so they could be extended without such a headache.

I see theme functions in there which is great, but sometimes when building a module that extends a module, using the theme layer just doesn't cut it.

Take it or leave it I'm just throwing it out there.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tmsimont’s picture

OK I'm attaching a rather substantial refactor patch. I hope that you consider this because I truly think that this kind of approach will make this module much more extendable for other module developers.

I think the current state of the calendar_plugin_style object is pretty non-extendable. The fact that we have OOP here is awesome and I think that right now the power of OOP is not being utilized to the fullest degree.

In this patch, I've started to break down the clunky calendar_plugin_style object by branching off the "year/month/week/day" build functions into their own objects that extend a new "calendar builder" class.

Rather than using a switch at the end of an already long function to switch between a set list of functions, I've introduced an overridable function called get_builder() that looks for a calendar_builder extension. This takes the heavy calendar building functions out of the plugin and opens the door to quick custom overrides of the get_builder() function.

-    switch ($this->options['calendar_type']) {
-      case 'year':
-        $rows = array();
-        $this->view->date_info->mini = TRUE;
-        for ($i = 1; $i <= 12; $i++) {
-          $rows[$i] = $this->calendar_build_mini_month();
-        }
-        $this->view->date_info->mini = FALSE;
-        break;
-      case 'month':
-        $rows = !empty($this->date_info->mini) ? $this->calendar_build_mini_month() : $this->calendar_build_month();
-        break;
-      case 'day':
-        $rows = $this->calendar_build_day();
-        break;
-      case 'week':
-        $rows = $this->calendar_build_week();
-        // Merge the day names in as the first row.
-        $rows = array_merge(array(calendar_week_header($this->view)), $rows);
-        break;
-    }
+    $builder = $this->get_builder();
+    $rows = $builder->build_rows();
     // Send the sorted rows to the right theme for this type of calendar.
     $this->definition['theme'] = 'calendar_' . $this->options['calendar_type'];
@@ -359,263 +340,443 @@ class calendar_plugin_style extends views_plugin_style {
     unset($this->view->row_index);
     return $output;
   }
+  
+  function get_builder() {
+    $builder_type = 'calendar_builder_' . $this->options['calendar_type'];
+    return new $builder_type($this);
+  }
+}

The builder extensions then handle the heavy data processing for the calendar views.

I've tested this with a basic install and there's no new functionality, just a much easier to work with foundation of code.

I think this could still use a lot of work... There are a lot of really confusing variable names, do/whiles and for loops that start to get pretty thick to understand. I think that the smaller we can make each chunk of code, the easier this will be to understand and the easier it will be to extend into bigger better things.
(http://blog.millermedeiros.com/keep-your-modules-and-functions-small/)

Anyway, I hope you consider committing this patch.. Thanks for all of the work you've done so far.

tmsimont’s picture

Status: Active » Needs review
tmsimont’s picture

attached revised patch without whitespace issues

tmsimont’s picture

missed iehint in above patches and had a few erroneous arguments. please disregard previous patches and use this one

tmsimont’s picture

I revised it again.. not going to put up an interdiff because I dont think anyone has looked at my other patches.. please disregard previous patches.

The most substantial changes here are in the month builder. The month building process is now broken up into 8 functions that together execute the same code that was in the 300 line single function. The difference here is that anyone that extends any one of these objects will have numerous points of entry into the process via override functions without worrying about re-writing too much code.

tmsimont’s picture

OK comprehensive refactor attached... no interdiff because again i feel like i'm talking to myself :)

tmsimont’s picture

dis one better. disregard previous.

An example extension of this in a module called "pattern" looks like this:


class pattern_plugin_style extends calendar_plugin_style {
  function get_builder() {
    $builder_type = 'pattern_builder_' . $this->options['calendar_type'];
    return new $builder_type($this);
  }
}

class pattern_builder_month extends calendar_builder_month {
  function get_week_builder() {
    return new pattern_builder_week($this->plugin);
  }
}

class pattern_builder_week extends calendar_builder_week {
  function get_day_builder($wday) {
    return new pattern_builder_day_weekday($this, $wday);
  }
}

class pattern_builder_day_weekday extends calendar_builder_day_weekday {
  function calendar_build_day() {
    parent::calendar_build_day();
    $this->week->add_singleday_bucket($this->wday, "BUCKET!");
  }
}

tmsimont’s picture

I've linked http://drupal.org/sandbox/tmsimont/1917664 back to this issue. This sandbox module uses the extensible calendar plugin that the patch in #7 creates. It's just one example of how the plugin could really be tweaked effectively to bring new and exciting functionality to drupal.

TechnoTim2010’s picture

You're not just talking to yourself, I need to apply this patch :-)

jaxpax’s picture

Issue summary: View changes

This is a great module, thanks @tmsimont!

Neslee Canil Pinto’s picture

Status: Needs review » Closed (outdated)