Structure for Small Custom System

[VaderDark]

I am working on a set of custom features for a firm that deals with housing. They have a set of Buildings, each building has a set of Units, and each Unit has a set of Tenants. Aside from storing this data, the staff stores Activities for Units and Tenants, such as "Mail Received to Building" for a certain Tenant and then "Mail Delivered to Tenant". Or "Maintenance Issue" for a unit (if the radiator breaks, for example).

The system also prints out a set of reports for the staff, like Building Status, Mail Status etc.

The system is actually being built in Drupal 9 and the data is stored in CiviCRM (a CRM module that runs inside of Drupal).

We have thus far 29 "forms" meaning 29 different pages the staff can visit to record activities or generate reports. In Drupal, each form requires it's own file.

I need a large set of helper functions that different form files need, such as getTenantsByUnit or recordActivity. I have those in one file, as one large class.

The issue is that that file is becoming large. It has 113 public functions and the file is 2600 lines long. How do you determine when it's time to break things up into smaller files? Perhaps via interfaces or Traits (multiple inheritance)? Or perhaps it's OK as it is.

There are a few other files, but 90% of this system is those 29 forms and this one helper file. We are adding more features and more forms however.

[requinix]

When you say helper "functions", do you mean actual functions or do you mean classes and class methods? How about a sample of what we're talking about?

[VaderDark]

I mean class methods. Here is an example where we create an instance of the class, and then use it to create a "Note" activity, and then we mark that Activity as "completed":

      $ML_Funcs = new MartinLutherFunctions();
      $activity_id = $ML_Funcs->createNote($x[1],$notesDetails);
      $ML_Funcs->markActivityCompleted($activity_id);

There you see "createNote" and "markActivityCompleted" -- those are two methods of the MartinLutherFunctions class. There are now 116 such methods (added 3 since last post).

Thank you.

[requinix]

Then the first step I would recommend towards improving the code is to split the functions into classes for categories. Like one for notes that has createNote. And another for activities that has markActivityCompleted. And unless you need actual instances of these classes, you might as well switch to purely static methods.

$activity_id = NotesFunctions::createNote($x[1],$notesDetails);
ActivityFunctions::markActivityCompleted($activity_id);

That alleviates the short-term problem of where you put code.

After that you could switch more easily to an actual OOP design, where you have a class for notes and a class for activities, and you use each instance of the class like it was a particular note/activity. As a simple example,

$note = Note::create($x[1], $notesDetails);
// $note is an instance of Note

/*
class Note {
	public function __construct($note_data) { ... }
	public static function create($whatever_x_is, $details) { ... return new Note($note_data); }
}
*/

$activity = $note->getActivity();
// $activity is an instance of Activity

/*
class Note {
	private $activity_id;
	public function getActivity() { return Activity::get($this->activity_id); }
}

class Activity {
	public function __construct($activity_data) { ... }
	public static function get($id) { ... return new Activity($activity_data); }
}
*/

$activity->markCompleted();

/*
class Activity {
	public function markCompleted() { ... }
}
*/

You can bridge the gap between those two steps by keeping NotesFunctions and ActivityFunctions, but changing their methods to use these new classes.

class NotesFunctions {
	public static function createNote($whatever_x_is, $details) {
		$note = Note::create($whatever_x_is, $details);
		return $note->getActivityId(); // getActivityId is a temporary method created so you can return an ID from here
	}
}

class ActivityFunctions {
	public static function markActivityCompleted($id) {
		$activity = Activity::get($id);
		$activity->markCompleted();
	}
}

 

[VaderDark]

Interesting ideas. Some are not entirely relevant, as I have only revealed a lines out of over 9K lines of code (across all the files).

But my initial question really was how do you determine when it's time to break things up into smaller files?

I am uncertain if it's necessary at this point, or if it will really help make things clearer and easier to work with going forward.

Thank you.

[requinix]

8 minutes ago, VaderDark said:

But my initial question really was how do you determine when it's time to break things up into smaller files?

You already know the answer to that: some point before you end up with 116 functions in a 2600+ line file.

As for where "before" is, that's tricky. Because despite what many others will say, sometimes having complicated things contained in one single place makes them easier to understand than if they had their components spread out across multiple locations - but having that as an exception to the rule also lends itself to being an excuse to ignore the rule.

The simplest approach is what I hinted at: separate by category, or purpose. With 116 functions there are going to be a much smaller number, perhaps a dozen, of groupings. Like notes functions, and activity functions. Even if you only had one "createNote" note function, the fact that it's a different subject matter than activities warrants having it in its own place.
The other answer is that good code design won't let you create massive utility classes to begin with - no offense. Following principles like SOLID (its "single-responsibility principle" is what that whole categorization thing is essentially about) or DRY will naturally encourage you to break things up as a side-effect.

[VaderDark]

Thank you very much for your feedback. You have given me some valuable direction.

[gizmola]

I think that requinix gave you some very good abstract advice.

Stepping back from this, a big part of your conundrum, without a doubt, was the choice to create a real estate portfolio management application in Drupal, where Drupal is not providing you the vast majority of value.  Drupal is a CMS, and its value proposition is based upon people that have a variety of media (text, images, video, documents) they want to categorize and organize (taxonomy) for the purposes of publishing information in a variety of ways.

I have no idea why Drupal was chosen in this circumstance.  It's no different than when people start with wordpress, then build out a whole custom, because the system will have a blog page.

Either symfony or laravel would have been a much more appropriate starting point in my opinion.  While it sounds like it's far too late to change course for you, that doesn't mean that recognition of the disconnect isn't worth recognition, because you can sometimes look at the gap, and build in thing to fill the gap using models from frameworks and perhaps orm's like Doctrine.

I saw this portion of your original post"  

Quote

getTenantsByUnit or recordActivity. I have those in one file, as one large class.

So starting with getTentantsByUnit, if you were working with a standard MVC framework like symfony, it's clear using doctrine that this the type of thing that you would have in a Doctrine "Repository" class.  Essentially, a repository class is associated with a "model" (aka a class associated with a database table) that can be used to make queries against that model.

One of the values of an orm is also that you are able to define the underlying relationships between models (which again map to tables) and get certain built in behaviors when querying, like in this case, querying for a unit and getting all the related tenant rows in the result.

The name says to me, that this is a function that should be in a class associated with the "Unit" table/model.  Doctrine implements the "Data mapper" pattern, as opposed to the "Active Record" pattern implemented by many other frameworks most notably for php, by laravel eloquent, or famously by Ruby on Rails.  

I personally prefer the Data Mapper pattern, and the key thing to recognize is that these are "patterns" and even though you aren't using an ORM, doesn't mean you can't implement your own version of these patterns.  As an example, I worked for a consumer service company that had a large legacy laravel code base, and even though they started with Laravel, over time, developers had essentially added classes to implement the "Repository" pattern on top of Eloquent.  

"recordActivity" sounds more to me like a "service" that you would create.  One of the major engineering decisions of Drupal, was when they rebuilt it using the symfony components as a starting point, so that it would be a dependency injection framework, and support integration and extensibility with php components.  This means that you can "wire" and/or configure your own services and use them in your application.  Perhaps you already know this, so please excuse this digression if you do.  Here's a quick pointer to some discussion of that:  https://www.drupal.org/docs/drupal-apis/services-and-dependency-injection/structure-of-a-service-file

So the idea behind this is that you use interfaces to describe your core dependency objects  (things for database manipulation, mailers, logging etc.)  and if you need that service, the DI container is able to instantiate the service at runtime, and you will be able to use it, taking advantage of configuration, lazy loading etc. rather than having to do all the setup work yourself.  This is another reason why you want to separate things into appropriate classes, since you are using Drupal and Drupal is now a Dependency injection framework at its core.  To do all this you want logical organization of classes using dependency injection principles, and you want to use interfaces where appropriate, or at the very least, typing of classes that will be inject into other classes or services as dependencies.

You also mentioned traits, and traits should by used sparingly and only when you 100% understand and can justify why you need them.  Typically this is to merge code into a class that is highly generic across a lot of classes.  The classic example of this, is having a code base, where you generically want some logging capabilities.  Since PHP is a single inheritance model, that is the way to go.

Just a throw away thought for you, but lately there's been a lot of talk by PHP developers about declaring most of your top level class methods as "final", and in many cases the class itself as final, so that it won't be used in inheritance when that ends up being a kludge.  I'd throw in that practice.  Here's a nice blog post by a symfony/doctrine core developer who has written a huge amount of code for those projects on the practice:  https://ocramius.github.io/blog/when-to-declare-classes-final/

[VaderDark]

This system is not about real estate. It's for a temporary housing institution, and the main issues are dealing with the tenants, not the properties.

Thank you for your post, it has a lot of good information.

One detail you didn't notice, however, is that this system is using CiviCRM. I realize that CiviCRM is not well known, but it's a CRM tool that runs inside of Drupal (or WordPress) and thus the concepts of Contacts and Relationships between them is already available there. We have 3 Contact Types:

Building
Unit
Tenant

and they share Relationships between each other (as in Unit 1A is "Unit Of" Building 123 Oak St.).

Other concepts such as Notes and Activities are also available in CiviCRM.

Some of the functions I wrote are, more or less, just wrappers are the CiviCRM API.

One issue, that we didn't foresee, is that the CiviCRM API is very slow. As our database grows, it's becoming too slow to use that for day to day activities. So we are implementing now a few custom tables to mirror just the current status of all Tenants and Units, and those tables are very simple and very fast to access. The full history of all Activities and Relationships remains in the CiviCRM records.

Given that this changes a lot of the code, I am taking this opportunity to refactor, and that was part of the reason I posted. I also appreciate the chance to discuss this as I work alone -- freelance and maverick style.

Anyhow, now I am creating these classes, to begin with:

Building
Unit
Tenant

and I am trying to make this actually object-oriented, this time around.

First build was in a rush and every new feature was news to me -- it was truly built on the fly, but the basic features they really needed are in place and working. Now I have a bit of time/space to rewrite this to make it faster, and also easier to build upon for the rest of the features.

Thank you both for the input. I really appreciate it.

[gizmola]

Sounds like an interesting project and some unique performance challenges.  Thanks for providing the update.   The main takeaway I hope you got from my reply, was that Dependency Injection is a pattern, that you can employ and perhaps benefit from, and of course Drupal already supports Services and does Dependency Injection.