Skip to content
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

global.R, server.R, and ui.R need to be refactored for correctness and readability #1

Closed
bryce-carson opened this issue May 18, 2024 · 7 comments
Assignees
Labels
🍝 spaghetti with tomato sauce UNSTABLE The work is to be completed in the unstable branch

Comments

@bryce-carson
Copy link
Collaborator

bryce-carson commented May 18, 2024

The server and UI code is chaotic and not written in a clear style using the syntax of R to its fullest. The server and UI code needs to be refactored so that it is more maintainable after I leave the project.

spatialEpisim/server.R

Lines 846 to 865 in df70041

if(input$modelSelect == "SEIRD"){
if (input$selectedCountry == "Czech Republic"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "CZE" & model == "SEIRD")[1,"lambda"])
} else if (input$selectedCountry == "Nigeria"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "NGA" & model == "SEIRD")[1,"lambda"])}
else if (input$selectedCountry == "Uganda"){
lambdaValue <- 5}
else if (input$selectedCountry == "Democratic Republic of Congo"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "COD" & model == "SEIRD")[1,"lambda"])}
} else if (input$modelSelect == "SVEIRD"){
if (input$selectedCountry == "Czech Republic"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "CZE" & model == "SVEIRD")[1,"lambda"])
} else if (input$selectedCountry == "Nigeria"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "NGA" & model == "SVEIRD")[1,"lambda"])
}
else if (input$selectedCountry == "Uganda"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "COD" & model == "SVEIRD")[1,"lambda"])}
else if (input$selectedCountry == "Democratic Republic of Congo"){
lambdaValue <- as.numeric(filter(epiparms, ISONumeric == "COD" & model == "SVEIRD")[1,"lambda"])}
}

If the names of the countries are added as a new column to the epiparms data file, these lines can be simplified to the following pipeline.

lambdaValue <-
filter(epiparms, model == input$modelSelect, country == input$selectedCountry)[1, "lambda"] |>
as.numeric()
@bryce-carson
Copy link
Collaborator Author

This is just an example of what can be improved in server.

bryce-carson added a commit that referenced this issue Jun 18, 2024
The mainPanel is now broken up into visualizer and simulator ui
objects. Height and width arguments are removed from the various image
and plot outputs to simplify the ui code, and to transfer responsibility
of the sizing of elements to CSS. Data assimilation UI inputs are now
defined in UI. Helpers added to more elements. Compartment-influencing
parameters are now defined in UI. More ui elements are defined in the UI
file rather than server; the whitespace and arguments are cleaned up,
and some objects are rearranged for clarity and intention.

The quotation marks in the ramp object in global.R are now double
quotes, per R-programming conventions (single quotes are valid, but
unpopular). Concatenated the "appCSS" object; there were three different
CSS rules assigned one after the other to the same object, but only the
latest would be used. An "acceptedFileTypes" global variable is
introduced to be used anywhere a data upload is used in the UI or
server. The "updateNumericInputs" function is borrowed from episim to
simplify how default input widget values are defined and set. Commentary
changes and whitespace changes.

Objects related to the non-spatial application mode are removed from
server.R, e.g. input validators.
Observable for the go button is made into a single line.
Terra output image code is cleaned up.
Commented on the Level1Country reactive value to ensure that I inspect
this spaghetti code closer.
Much unnecessary commentary is removed.
UI input widget outputs are removed in most cases, and defined in the UI
file (as they should've been from the start; fix #1).
Input widgets for uploaded observed-data assimilation are defined
programmatically.
Recommended aggregation factor reactive introduced to simplify other
code.
"Main" functionality triggered when go button is clicked is cleaned up
and improved for readability and correctness.

"include/authors.md" added to simplify some UI code. There's no reason
for it to be defined in the UI file, really.

"misc/epiparms.xlsx" updated to ensure column names are input widget ids
to permit updatedNumericInputs to work correctly.
@bryce-carson bryce-carson added the RESOLVED_IN_BRANCH This issue will automatically close when a branch is merged. label Jul 5, 2024
@bryce-carson bryce-carson self-assigned this Jul 17, 2024
@bryce-carson bryce-carson added 🍝 spaghetti with tomato sauce UNSTABLE The work is to be completed in the unstable branch labels Jul 17, 2024
@bryce-carson
Copy link
Collaborator Author

This is just an example of what can be improved in server.

To justify this issue being an example I am labelling it duplicate of the less specific #32 which represents the needed work to refactor the entire application architecture and implementation.

@bryce-carson bryce-carson added the duplicate This issue or pull request already exists label Jul 18, 2024
@bryce-carson bryce-carson changed the title Simplify update of lambdaValue Example of refactoring work needed: simplify update of lambdaValue Jul 18, 2024
@bryce-carson bryce-carson changed the title Example of refactoring work needed: simplify update of lambdaValue server.R and ui.R need to be refactored for correctness and readability Jul 18, 2024
@bryce-carson bryce-carson removed the duplicate This issue or pull request already exists label Jul 18, 2024
@bryce-carson
Copy link
Collaborator Author

I removed the duplicate label and renamed the issue to make it clearer that this issue was originally opened to track the refactoring of the server and UI code. At the time I opened this issue I was unaware that the contents of the R/ sub-folder would need to be refactored as well, so this issue tracks a separate but linked refactoring.

The server code will take advantage of the functionality provided by the functions defined in one or more files in the R/ sub-folder, so this issue is related to the refactoring occurring in #32 in that way and they will be closed at the same time when the unstable branch is merged in to main.

@bryce-carson bryce-carson removed the RESOLVED_IN_BRANCH This issue will automatically close when a branch is merged. label Jul 18, 2024
@bryce-carson
Copy link
Collaborator Author

As of today

Before

[bryce@fedora]~/Documents/src/r/spatialEpisim% scc --no-cocomo ~/src/r/spatialEpisim.stable/server.R 
───────────────────────────────────────────────────────────────────────────────
Language                 Files     Lines   Blanks  Comments     Code Complexity
───────────────────────────────────────────────────────────────────────────────
R                            1       792      129        53      610         84
───────────────────────────────────────────────────────────────────────────────
Total                        1       792      129        53      610         84
───────────────────────────────────────────────────────────────────────────────
Processed 28001 bytes, 0.028 megabytes (SI)
───────────────────────────────────────────────────────────────────────────────

After

[bryce@fedora]~/Documents/src/r/spatialEpisim% scc --no-cocomo server.R                             
───────────────────────────────────────────────────────────────────────────────
Language                 Files     Lines   Blanks  Comments     Code Complexity
───────────────────────────────────────────────────────────────────────────────
R                            1       614       86        81      447         50
───────────────────────────────────────────────────────────────────────────────
Total                        1       614       86        81      447         50
───────────────────────────────────────────────────────────────────────────────
Processed 23659 bytes, 0.024 megabytes (SI)
───────────────────────────────────────────────────────────────────────────────

@bryce-carson bryce-carson changed the title server.R and ui.R need to be refactored for correctness and readability global.R, server.R, and ui.R need to be refactored for correctness and readability Aug 13, 2024
@bryce-carson
Copy link
Collaborator Author

Global

Global needed to be cleaned up, so that the libraries that were loaded were only those that are actually used in the application and all of its dependencies. Files below the R/ folder, of course, should not be responsible for loading their own dependencies; it is difficult to track and maintain the files when that is done. It makes it difficult to study too.

The meat and potatoes of the application---its foundational SVEIRD simulation feature---is now documented and refactored in the package spatialEpisim.foundation. This will be hosted in a drat repository, and can then be installed quite easily by users (similarly to install_github from the devtools package).

Excerpt

spatialEpisim/global.R

Lines 1 to 42 in f5a7913

options(conflicts.policy = list(warn = FALSE),
scipen = 999)
## FIXME: I have a strong feeling not all of these libraries are actually used
## in spatialEpisim. TODO: remove all packages which are dependencies of
## spatialEpisim.foundation; there's no need to load them twice.
suppressPackageStartupMessages({
library(av)
library(bslib)
library(cptcity)
library(countrycode)
library(deSolve) # ::solve?
library(DT)
library(leaflet)
library(maps)
library(plotly)
library(readxl)
library(writexl)
library(terra)
library(shiny)
library(shinyalert)
library(shinybusy)
library(shinyhelper)
library(shinyjs)
library(shinyvalidate)
library(shinyWidgets)
library(tidyverse)
library(here)
})
## TODO: include instructions in the README on installing the following package;
## it's quite easy. DONT suppress messages from our own backend dependency.
library(spatialEpisim.foundation)
library(reactChartEditor)
here::i_am("global.R")

@bryce-carson
Copy link
Collaborator Author

UI

The user interface, of course, was messy and has been rewritten. Things which were in server.R and didn't need to be, either because they could be refactored to not need to be, or just didn't belong there at all in any case, were moved to UI were they belonged.

@bryce-carson
Copy link
Collaborator Author

Server

See the previous comment.

Code which was written by chat bots, or which didn't perform the documented action have been rewritten for clarity and maintainability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍝 spaghetti with tomato sauce UNSTABLE The work is to be completed in the unstable branch
Projects
None yet
Development

No branches or pull requests

1 participant