-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Determine emptier car location and crowding description #677
Conversation
Coverage of commit
|
Coverage of commit
|
lib/content/message/predictions.ex
Outdated
6 => [ | ||
train_level: {1, 6} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: This could be a place where pattern matching really shines. If we create a list that represents the status of each car, e.g. [:f, :e, :e, :f, :f, :f]
, we could pattern match against that, along with the number of empty cars, and create a series of cases like: {2, [_, _, _, :f, :f, :f]} -> :front
. That gives a nice visual way to express the logic, and would also allow us to fold in the special cases from the code below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah that's an interesting idea. I sort of like how the current approach condenses the logic but I can also see that it's a bit more obscure than expressing everything as patterns. Will give it a try and see how it turns out
end | ||
|
||
defp get_emptier_location(car_crowding_levels) do | ||
case car_crowding_levels do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@panentheos Let me know what you think of this approach. With the number of cases laid out I find it a little hard to read, but I do like how it simplifies the code in the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. It's very dense, but everything is laid out so it's very traceable. Also the simplification of the caller is pretty huge.
Coverage of commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment, then looks good!
lib/content/message/predictions.ex
Outdated
{2, [:f, :f, _, :f, :f, _]} -> :back | ||
{2, [:f, :f, _, _, :f, :f]} -> :middle | ||
{2, [:f, _, :f, _, :f, :f]} -> :middle | ||
{2, [:f, :f, _, :f, _, :f]} -> :middle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like 2
needs case(s) for :front_and_back
. Check my logic, but I think we could collapse things a bit by relying on the fact that these are checked in order, so [_, _, :f, :f, _, _]
would suffice as long as it came after :front
and :back
. Similarly, the :middle
cases could be handled with just [:f, _, _, _, _, :f]
as the last check (or just use a default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch. I meant to add a default to catch the :front_and_back
case but must've skipped it. I think you're right though about collapsing. Will check the logic on that and update the PR and merge if all looks good
Coverage of commit
|
Summary of changes
Asana Ticket: 🟠🧑🤝🧑🔊 Crowding Message Selection Logic
This PR adds logic that calculates the emptier car locations and crowding description. These pieces will later be used to
create the full audio message that will get tagged onto either the Arrival or Approaching message.
The "crowding zone" logic is depicted in this diagram