pass local values in bulk in bus code #789
Conversation
Coverage of commit
|
lib/signs/bus.ex
Outdated
| @@ -168,22 +178,24 @@ defmodule Signs.Bus do | |||
| |> filter_predictions(current_time, state) | |||
| |> Enum.group_by(&{&1.stop_id, &1.route_id, &1.direction_id}) | |||
|
|
|||
| locals = %{ | |||
There was a problem hiding this comment.
I'm trying to think of a more descriptive name for this... "locals" sort of tells you what its function is, but not what it is. Maybe something like MessageContext — it's context used to create sign messages.
There was a problem hiding this comment.
I went back and forth on this a bunch. Ultimately it's not just "messages", but also "announcements", and perhaps other things. context works, but it's really generic. locals felt decent because it describes the lifecycle of the data, without being prescriptive about what's in there.
There was a problem hiding this comment.
I think part of the reason I'm not a fan of locals is because it implies "local variables", which are a specific thing, and this isn't quite that thing (you could say it's... non-local variables?). context, while similarly generic, doesn't have this overlap. But don't feel like you have to change it on my preference alone 🙂
lib/signs/bus.ex
Outdated
| state | ||
| ) do | ||
| @spec static_text_content(locals(), t()) :: content_values() | ||
| defp static_text_content(%{config: config} = locals, state) do |
There was a problem hiding this comment.
What do you think of extracting the functions which accept this "locals" value into their own module? Sort of like a "view" for which the main module is the "controller". I think having some split of responsibilities here would make both parts easier to understand (and easier to test?) vs. all being in one mega-module, even if the "view" logic is still a high proportion. (I'm not sure off-hand how much of the state value the "view" functions use, but maybe the parts that are needed could be copied into the "locals" value to reduce coupling between the modules.)
There was a problem hiding this comment.
Part of the motivation for streamlining this was to avoid the temptation to move things around. By their nature, these functions are very tightly coupled together with the run_loop, via the data flow (which is why this pattern works at all). Moving some of them to another file would just mean having to jump around to multiple places to follow the logic. By the same token, the particular function boundaries are kind of arbitrary, so I'd hesitate to crystallize those internal APIs with individual test cases. The fact that the bus tests all exercise the full run_loop is what made it possible to rearrange the guts without having to rewrite a bunch of tests as well.
Coverage of commit
|
Summary of changes
The sign update loop follows a common pattern of:
The message algorithm is fairly complex, so it's broken into several functions to make it easier to manage. However, this requires passing the relevant data through those functions, which can be verbose, especially in the bus code.
This introduces a map of "locals" to contain the relevant data, which is then passed through the function tree in bulk. The idea is similar to the
conndata structure from thepluglibrary, acting as an accumulator for temporary, contextual data. Call sites are significantly cleaner, and destructuring in the function definitions provides an indication of what's actually in use at each level.