search

ashokkrish / spatialEpisim

spatialEpisim: Spatial Tracking of Infectious Disease Epidemics using Mathematical Models
GNU General Public License v3.0
9 stars 4 forks source link

R/ subfolder is full of spaghetti #32

Closed bryce-carson closed 3 months ago

bryce-carson commented 4 months ago

🍝 The spaghetti should be refrigerated, and then baked into a leftover-spaghetti-caserolle; see the branch of that name for progress.

ashokkrish commented 4 months ago

Spaghetti alle vongole? Spaghetti aglio e olio? Spaghetti alla carbonara? :)

bryce-carson commented 4 months ago

Spaghetti alle vongole? Spaghetti aglio e olio? Spaghetti alla carbonara? :)

I haven't heard of any of those, honestly! 😆 Spaghetti code, unfortunately! Maybe pizza code; I'm unsure. They're not really useful terms.

In short, the code is too interdependent to justify splitting it into so many files, and no single file has a clear role other than to call functions from another file, and so on, until some input data is finally needed. I can't test them in isolation or outside of the Shiny app.

I reviewed the function call diagram you shared with me, and I found that it is inaccurate. Some scripts call functions from files that were not listed as dependencies on the graph, and overall these files really aren't independent.

As they sat, they posed a problem for cleaning up the global.R file. Shiny already sources all R files under the R/ folder, so source calls aren't necessary. The source calls and further calls to library in the files underneath R/ make things even more complex and fragile, and prone to breakage.

It'll get better, slowly. Maybe a week to get through all of these files? I hope by the end of Wednesday, actually, I can have these refactored into a single package which will work independently of the Shiny app, and will have plenty of documentation that we can work on together to ensure that it is useful and testable (in isolation from Shiny). Real modularization.

bryce-carson commented 4 months ago

See the mention of this issue in the storyline above this comment.

bryce-carson commented 3 months ago

Given only #43 is still open, this issue will be closed as it doesn't serve any extra purpose beyond tracking that now.