Analytics duplication refactor #4920
Conversation
hmda-analytics/src/main/scala/hmda/analytics/HmdaAnalyticsApp.scala
Outdated
Show resolved
Hide resolved
hmda-analytics/src/main/scala/hmda/analytics/HmdaAnalyticsApp.scala
Outdated
Show resolved
Hide resolved
hmda-analytics/src/main/scala/hmda/analytics/HmdaAnalyticsApp.scala
Outdated
Show resolved
Hide resolved
hmda-analytics/src/main/scala/hmda/analytics/HmdaAnalyticsApp.scala
Outdated
Show resolved
Hide resolved
| @@ -392,85 +379,85 @@ object HmdaAnalyticsApp extends App with TransmittalSheetComponent with LarCompo | |||
| .mapAsync(1) { lar => | |||
| for { | |||
| insertorupdate <- submissionId.period match { | |||
There was a problem hiding this comment.
Could we replace this very long match statement with a map? And maybe do the same for the other match statements here? It would cut down on a ton of lines of code
There was a problem hiding this comment.
Hmm could you clarify a bit what you were thinking?
There was a problem hiding this comment.
Never mind, map isn't the right thing here. Not sure what I was thinking yesterday.
What I meant is we can simplify this to 2-3 cases: case Period(year, None) => and case Period(year, Some(quarter)) => (and keep the final case _ => case). Using a variable for the year and quarter will get rid of a ton of these match cases.
There was a problem hiding this comment.
Oh yeah let's do that, will take some time to update
|
@PatrickGoRaft , what are your thoughts on this PR generally? I'm sure it can be made to work if there are any current functional problems, and I'm always for reusing code, but how many lines are we cutting out of the total code size? I wonder if we could do even better with.a 2D (year, quarter) array, if it's worth it. |
@tptignor I don't want to mix up work to much. Let's keep this PR with its current class structure that I wrote. A further refactor, with a complete redesign, ought to be another ticket. |
hmda-analytics/src/main/scala/hmda/analytics/query/TransmittalSheetComponent.scala
Show resolved
Hide resolved
hmda-analytics/src/main/scala/hmda/analytics/HmdaAnalyticsApp.scala
Outdated
Show resolved
Hide resolved
| @@ -317,85 +317,12 @@ object HmdaAnalyticsApp extends App with TransmittalSheetComponent with LarCompo | |||
| for { | |||
| insertorupdate <- submissionId.period match { | |||
| case Period(2018, None) => YearlyLarRepositoryWrapper("2018").getLarRepository.insert(LarConverter2018(lar)) | |||
There was a problem hiding this comment.
2018 has a special lar converter so we're keeping this here
| larRepository2024Q3.insert( | ||
| LarConverter(lar = lar, 2024, isQuarterly = true) | ||
| case Period(2018, None) => YearlyLarRepositoryWrapper("2018").getLarRepository.insert(LarConverter2018(lar)) | ||
| case Period(submissionId.period.year, None) => YearlyLarRepositoryWrapper(submissionId.period.year.toString).getLarRepository.insert( |
There was a problem hiding this comment.
Does using submissionId.period.year in the case work when you test it? I thought just declaring a new variable year would work here
There was a problem hiding this comment.
Yeah this works, on another unit test ticket I'm going to refactor this a bit to demonstrate that. I think we should stick to making this depend on the submissionId for now since this line is a part of reading from that.
I implemented repository wrapper classes to decrease the code duplication in the hmda analytics app