Refactor TimeSeries artifacts #651

2024-04-19T11:22:04+08:00

josefarias commented

2024-04-19 11:22:04 +08:00

(Migrated from github.com)

I did this refactoring as an exercise for getting oriented before tackling https://github.com/maybe-finance/maybe/issues/643 and, well, for practice.

I think the resulting code is more robust and easier to follow. Here's what I did:

Detangle responsibilities. All classes are still there, but they're less aware of each other's concerns, even if not perfect yet. For example, each class knows how to turn itself into json, and TimeSeries::Value knows how to calculate its own trend, rather than TimeSeries orchestrating all of it.
Make use of Ruby and Rails idioms to make the code a bit more terse and easier to follow to someone who would recognize these patterns.
Reindent a couple of files to 2 spaces, as is the Ruby convention.
Replace the notion of a TimeSeries type with favorable_direction. This new name is longer, but communicates exactly what it's for as opposed to "type" which could mean anything. This already uncovered a bug in the #trend_styles helper!
Remove some unused codepaths, like TimeSeries::Value knowing how to destructure an object when we're only ever passing hashes to it.
Removing duplicate knowledge like what the favorable_direction is. As far as I can tell, trends and values can only exist within a series — and it's the nature of the series which determines what the favorable_direction is. So trends now delegate that knowledge to its containing series. This is easily changed if we ever want the two to diverge.

I'm mostly submitting this PR because I did the work recreationally and I'm presenting it for what it's worth. There's more work to be done here (two big smells are how often we're calling #is_a? and how often we explicitly need to handle nil). But I think this is a step in the right direction and I had to draw the line somewhere.

I think this would be an improvement that makes the campsite cleaner. But the value of a refactor is subjective and I'm happy to close this if this is not something that warrants focus right now. Just didn't want to throw away the work when it might (or might not) be valuable.

All tests are passing. If this breaks anything I'm sure it'd be an easy fix. We're at a stage where we can make this kind of change, I think.

I did this refactoring as an exercise for getting oriented before tackling https://github.com/maybe-finance/maybe/issues/643 and, well, for practice. I think the resulting code is more robust and easier to follow. Here's what I did: 1. Detangle responsibilities. All classes are still there, but they're less aware of each other's concerns, even if not perfect yet. For example, each class knows how to turn itself into json, and `TimeSeries::Value` knows how to calculate its own trend, rather than `TimeSeries` orchestrating all of it. 2. Make use of Ruby and Rails idioms to make the code a bit more terse and easier to follow to someone who would recognize these patterns. 3. Reindent a couple of files to 2 spaces, as is the Ruby convention. 4. Replace the notion of a TimeSeries `type` with `favorable_direction`. This new name is longer, but communicates exactly what it's for as opposed to "type" which could mean anything. This already uncovered a bug in the `#trend_styles` helper! 5. Remove some unused codepaths, like `TimeSeries::Value` knowing how to destructure an object when we're only ever passing hashes to it. 6. Removing duplicate knowledge like what the `favorable_direction` is. As far as I can tell, trends and values can only exist within a series — and it's the nature of the series which determines what the `favorable_direction` is. So trends now delegate that knowledge to its containing series. This is easily changed if we ever want the two to diverge. I'm mostly submitting this PR because I did the work recreationally and I'm presenting it for what it's worth. There's more work to be done here (two big smells are how often we're calling `#is_a?` and how often we explicitly need to handle `nil`). But I think this is a step in the right direction and I had to draw the line somewhere. I think this would be an improvement that makes the campsite cleaner. But the value of a refactor is subjective and I'm happy to close this if this is not something that warrants focus right now. Just didn't want to throw away the work when it might (or might not) be valuable. All tests are passing. If this breaks anything I'm sure it'd be an easy fix. We're at a stage where we can make this kind of change, I think.

zachgoll commented

2024-04-19 20:35:23 +08:00

(Migrated from github.com)

@josefarias thanks for the improvements here, I think these are certainly worth merging into main.

While I know this doesn't change the public interface, I'll wait until the bounty is over to merge these changes so as not to disrupt any others working with this code right now. If you end up submitting a bounty solution, you're more than welcome to include these changes if it makes things easier. Up to you.

RE the changes: Overall, I agree with the changes. I have a few ideas around the naming of Trend.direction and copious is_a? calls and nil checks. Would you be okay if I opened a PR against your branch with some ideas?

@josefarias thanks for the improvements here, I think these are certainly worth merging into `main`. While I know this doesn't change the public interface, I'll wait until the bounty is over to merge these changes so as not to disrupt any others working with this code right now. If you end up submitting a bounty solution, you're more than welcome to include these changes if it makes things easier. Up to you. RE the changes: Overall, I agree with the changes. I have a few ideas around the naming of `Trend.direction` and copious `is_a?` calls and `nil` checks. Would you be okay if I opened a PR against your branch with some ideas?

josefarias commented

2024-04-19 21:30:00 +08:00

(Migrated from github.com)

@zachgoll thanks for taking a look! Of course, feel free to open a PR against this branch.

Waiting until the bounty is over makes sense 👍

@zachgoll thanks for taking a look! Of course, feel free to open a PR against this branch. Waiting until the bounty is over makes sense 👍

zachgoll commented

2024-04-22 20:30:33 +08:00

(Migrated from github.com)

@josefarias to avoid blocking the bounty PR and to align with your suggestions, I'm going to go ahead and merge this as-is. I think these changes are good improvements to the existing design, so no reason to block a merge on them.

I will tag you in any follow-up PRs that I open in relation to this.

@josefarias to avoid blocking the bounty PR and to align with your suggestions, I'm going to go ahead and merge this as-is. I think these changes are good improvements to the existing design, so no reason to block a merge on them. I will tag you in any follow-up PRs that I open in relation to this.

👍 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#651

Refactor TimeSeries artifacts #651

Refactor `TimeSeries` artifacts #651