Use class instances for common objects handled in image loading optimization logic

[felixarntz]

Currently all logic is happening in functions, which in a few places makes the code complicated to follow. Specifically the handling of data arrays could be simplified, and potentially in a few places be made more efficient (e.g. by storing things in a class property rather than recalculating again). See various comments on unit test PR.

Here's an initial proposal:

• Introduce class to represent a single URL metric (e.g. ILO_URL_Metric).
  • This would mostly be a value object with properties like url, slug, viewport, elements.
  • The class could potentially have private logic to take care of validation.
  • The elements entry may deserve some further thought, particularly around how it could work considering additional use-cases than LCP elements in the future.
• Introduce class to represent a grouped URL metrics collection by breakpoint (e.g. ILO_Grouped_URL_Metric_Collection).
  • The class could receive a list (indexed array) of ILO_URL_Metric instances and then take care of grouping them accordingly. The grouping internally could just be a map of breakpoint => ILO_URL_Metric[].
  • The class should probably implement Traversable, so that it can be iterated just like an array (over the breakpoints).
  • The class could have a method unshift( ILO_URL_Metric $metric ) that replaces the ilo_unshift_url_metrics() function and integrates the given metric properly, while discarding excess metrics beyond the configured sample size at the same time.
  • Some of the grouping "configuration" (e.g. breakpoints, sample size) may also be useful to provide via constructor.
• The ilo_group_url_metrics_by_breakpoint() function could then use these two classes.
  • It seems the module currently always uses the breakpoints from ilo_get_breakpoint_max_widths() and the sample size from ilo_get_url_metrics_breakpoint_sample_size(). If so, we could probably avoid those function calls elsewhere and only call them in ilo_group_url_metrics_by_breakpoint(). In other words, only the group class constructor would take them as parameters (for flexibility and testing), but the wrapper function to instantiate the group class wouldn't, it would just use the default ones, to avoid having to pass those parameters around in several places.
• Any functions that currently expect a $grouped_url_metrics array should be updated to expect an instance of the new group class. If there are particular usages where another method on that class would be helpful, it could be added based on need.
• Functions that currently expect a (non-grouped) $url_metrics array but then immediately group them without doing anything else to the original array should be modified to expect an instance of the new group class as well, avoiding another call to the grouping function. This also helps separating responsibilities.
• Functions that today return an array of URL metric associative arrays should going forward return an array of URL metric class instances (e.g. ilo_parse_stored_url_metrics()).
• We could consider storing the relevant post ID on URL metric instances (or their group instances) in the cases where the relationship to a specific WP post object is already established (i.e. some metrics for the relevant URL are already stored). This would simplify storing URL metrics, particularly on an update, because we wouldn't have to look up the correct post once again.

@westonruter There may be more than the above, but those are the things I consider most essential to simplify a couple things, and they'd be the most foundational ones (basically use objects for things that are commonly passed around and aren't just simple scalar values). Please let me know your thoughts on the above proposal.

[felixarntz]

@westonruter Related question: In the post content JSON, does it store the simple list of URL metrics, or does it store the grouped lists by breakpoint? Not entirely clear on that from looking at the code.

[westonruter]

@felixarntz It does not store groups. The grouping into breakpoints happens at runtime, since the breakpoints are configurable. So it stores a flat list. When a new URL metric is added, it will do the grouping only for the purpose of pushing out an older URL metric for the same breakpoint. But after the new URL metric is added, the groups are flattened again for storage.

[felixarntz]

@westonruter Thanks, makes sense. I think that doesn't really change much in my proposal above. We would probably want to have a method on ILO_Grouped_URL_Metric_Collection to return the simple ILO_URL_Metric[] representation of it (i.e. the indexed list), e.g. for the purpose of storing in that way after making modifications.