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.
Comments
Comment #1
tmsimont CreditAttribution: tmsimont commentedOK 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 theget_builder()
function.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.
Comment #2
tmsimont CreditAttribution: tmsimont commentedComment #3
tmsimont CreditAttribution: tmsimont commentedattached revised patch without whitespace issues
Comment #4
tmsimont CreditAttribution: tmsimont commentedmissed
iehint
in above patches and had a few erroneous arguments. please disregard previous patches and use this oneComment #5
tmsimont CreditAttribution: tmsimont commentedI 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.
Comment #6
tmsimont CreditAttribution: tmsimont commentedOK comprehensive refactor attached... no interdiff because again i feel like i'm talking to myself :)
Comment #7
tmsimont CreditAttribution: tmsimont commenteddis one better. disregard previous.
An example extension of this in a module called "pattern" looks like this:
Comment #8
tmsimont CreditAttribution: tmsimont commentedI'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.
Comment #9
TechnoTim2010 CreditAttribution: TechnoTim2010 commentedYou're not just talking to yourself, I need to apply this patch :-)
Comment #10
jaxpax CreditAttribution: jaxpax commentedThis is a great module, thanks @tmsimont!
Comment #11
Neslee Canil Pinto