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

More Handlers #187

Merged
merged 9 commits into from
Aug 8, 2024
Merged

More Handlers #187

merged 9 commits into from
Aug 8, 2024

Conversation

ayushsingh01042003
Copy link
Contributor

Description

  • EDDGridSideBySideHandler
  • EDDTableAggregateRowsHandler
  • EDDTableCopyHandler
  • EDDTableFromCassandraHandler
  • EDDTableFromDapSequenceHandler
  • EDDTableFromDatabaseHandler
  • EDDTableFromAsciiServiceHandler

Copy link

@7yl4r 7yl4r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Two thoughts:

  1. would a map be more readable than a switch statement for the mapping of handler dataset type strings to handlers in the HandlerFactory?
  2. anything we can do about the duplicated variables in each handler? eg:
  private ArrayList tDataVariables = new ArrayList();
  private int tReloadEveryNMinutes = Integer.MAX_VALUE;
  private String tAccessibleTo = null;
  private String tGraphsAccessibleTo = null;
  private StringArray tOnChange = new StringArray();
  private String tFgdcFile = null;

solutions to (2) might get into things better left alone like multiple inheritance or an entity-component pattern.

@ChrisJohnNOAA
Copy link
Contributor

Looks good to me. Two thoughts:

  1. would a map be more readable than a switch statement for the mapping of handler dataset type strings to handlers in the HandlerFactory?
  2. anything we can do about the duplicated variables in each handler? eg:
  private ArrayList tDataVariables = new ArrayList();
  private int tReloadEveryNMinutes = Integer.MAX_VALUE;
  private String tAccessibleTo = null;
  private String tGraphsAccessibleTo = null;
  private StringArray tOnChange = new StringArray();
  private String tFgdcFile = null;

solutions to (2) might get into things better left alone like multiple inheritance or an entity-component pattern.

There are definitely possible solutions to 2, but I'm not sure they're worth doing at this time. It's a lot of boilerplate with the current approach, but the solutions to remove the duplication would make the code much more complicated. With the current approach it's 1 line to declare the variable and 1 line to do the parsing.

My first thought on how to reduce the duplication is a configurable class that would require you to specify each variable and it's type (which covers parsing approach and what type to store it as) for the dataset. Then have that handle the parsing, but it would either need to handle every variable or we'd still need some logic for datasets that have special/non-standard variables (like child datasets). I also don't know if this would be significantly less code overall (because configuring the parser would be close to the current amount of code).

Copy link
Contributor

@ChrisJohnNOAA ChrisJohnNOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly comments on better typing which should allow us to avoid some of the loops.

case "dimensionValuesInMemory" -> tDimensionValuesInMemory = String2.parseBoolean(contentStr);
case "dataset" -> {
EDDGrid[] tcds = new EDDGrid[tChildDatasets.size()];
for (int c = 0; c < tChildDatasets.size(); c++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make the tChildDatasets of type ArrayList<EDDGrid>, you should be able to just use tChildDatasets.toArray() instead of a for loop here.

case "dataset" -> {
int nChildren = tChildren.size();
EDDTable[] ttChildren = new EDDTable[nChildren];
for (int i = 0; i < nChildren; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make the tChildDatasets of type ArrayList<EDDTable>, you should be able to just use tChildDatasets.toArray() instead of a for loop here.

case "addVariablesWhere" -> tAddVariablesWhere = contentStr;
case "dataset" -> {
int ndv = tDataVariables.size();
Object[][] ttDataVariables = new Object[ndv][];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above, you should be able to declare tDataVariables as ArrayList<Object[]> and then use tDataVariables.toArray() instead of the loop. (This also applies to the previous datasets that have data variables and axis variables.)

case "dataset" -> {
int ndv = tDataVariables.size();
Object[][] ttDataVariables = new Object[ndv][];
for (int i = 0; i < tDataVariables.size(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this loop.

case "dataset" -> {
int ndv = tDataVariables.size();
Object[][] ttDataVariables = new Object[ndv][];
for (int i = 0; i < tDataVariables.size(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and another instance

@ChrisJohnNOAA
Copy link
Contributor

Mainly comments on better typing which should allow us to avoid some of the loops.

The toArray method returns an Object[], not the specific type. There are a few ways to get a typed array (without a loop).

https://www.geeksforgeeks.org/arraylist-array-conversion-java-toarray-methods/

The best approach is probably their method 2, which looks like:

EDDGrid[] arr = new EDDGrid[tChildDatasets.size()];
arr = tChildDatasets.toArray(arr);

@ayushsingh01042003
Copy link
Contributor Author

Pushed the changes

@ChrisJohnNOAA ChrisJohnNOAA merged commit 4bc03bf into ERDDAP:main Aug 8, 2024
@ayushsingh01042003 ayushsingh01042003 deleted the SAXParser branch August 9, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants