88 - Optimised code around site stats page (#52)

* 88 - Optimised code around site stats page * 88 - Optimisations made to the code * [88]: Resolve some PR comments and new CodeQL notes * 88 - Refactored code into a smaller method to make it a bit more readable. * 88 - Updated Javadoc * 88 - Fixed some checkstyle issues --------- Co-authored-by: Robert Sayles <robert.sayles@and.digital>
isaacphysics · Sep 29, 2023 · d418e8f · d418e8f
1 parent 1851fb9
commit d418e8f
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 65 deletions.
diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IQuestionAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IQuestionAttemptManager.java
@@ -54,6 +54,8 @@ Map<String, Map<String, List<QuestionValidationResponse>>> getQuestionAttempts(L
       throws SegueDatabaseException;
 
   /**
+   * Registers an anonymous user's attempt to answer a question and stores it in the database.
+   *
    * @param userId          - some anonymous identifier
    * @param questionPageId  - question page id
    * @param fullQuestionId  - full question id
@@ -64,6 +66,8 @@ void registerAnonymousQuestionAttempt(String userId, String questionPageId, Stri
                                         QuestionValidationResponse questionAttempt) throws SegueDatabaseException;
 
   /**
+   * Retrieves the question attempts of an anonymous user from the database.
+   *
    * @param anonymousId - some anonymous identifier
    * @return List of questionpage --> question id --> list of QuestionResponses.
    */
@@ -81,13 +85,15 @@ void mergeAnonymousQuestionInformationWithRegisteredUserRecord(String anonymousU
       throws SegueDatabaseException;
 
   /**
-   * Count the users by role which have answered questions over the previous time interval.
+   * Retrieves a count of users by their roles who have answered questions within the specified time ranges.
    *
-   * @param timeInterval time interval over which to count
-   * @return map of counts for each role
-   * @throws SegueDatabaseException - if there is a problem with the database.
+   * @param timeIntervals An array of time ranges (in string format) for which to get the user counts.
+   *                      Each time range is used in the SQL query to filter the results.
+   * @return A map where the keys are the time ranges and the values are another map containing the count of users
+   *     for each role within that time range.
+   * @throws SegueDatabaseException If there is a database-related issue, such as a SQL exception.
    */
-  Map<Role, Long> getAnsweredQuestionRolesOverPrevious(TimeInterval timeInterval)
+  Map<TimeInterval, Map<Role, Long>> getAnsweredQuestionRolesOverPrevious(TimeInterval[] timeIntervals)
       throws SegueDatabaseException;
 
   /**

diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/PgQuestionAttempts.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/PgQuestionAttempts.java
@@ -16,7 +16,6 @@
 
 package uk.ac.cam.cl.dtg.isaac.quiz;
 
-import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval;
 import static uk.ac.cam.cl.dtg.segue.api.managers.QuestionManager.extractPageIdFromQuestionId;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
@@ -44,6 +43,7 @@
 import uk.ac.cam.cl.dtg.isaac.dos.LightweightQuestionValidationResponse;
 import uk.ac.cam.cl.dtg.isaac.dos.QuestionValidationResponse;
 import uk.ac.cam.cl.dtg.isaac.dos.users.Role;
+import uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval;
 import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException;
 import uk.ac.cam.cl.dtg.segue.dao.content.ContentMapper;
 import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb;
@@ -401,27 +401,36 @@ public void mergeAnonymousQuestionInformationWithRegisteredUserRecord(final Stri
   }
 
   @Override
-  public Map<Role, Long> getAnsweredQuestionRolesOverPrevious(final TimeInterval timeInterval)
-      throws SegueDatabaseException {
+  public Map<TimeInterval, Map<Role, Long>> getAnsweredQuestionRolesOverPrevious(final TimeInterval[] timeRanges)
+          throws SegueDatabaseException {
     String query = "SELECT role, count(DISTINCT users.id) FROM question_attempts"
-        + " JOIN users ON user_id=users.id AND NOT deleted WHERE timestamp > now() - ? GROUP BY role";
-    try (Connection conn = database.getDatabaseConnection();
-         PreparedStatement pst = conn.prepareStatement(query)
-    ) {
-      pst.setObject(FIELD_GET_ANSWERED_QUESTION_ROLES_INTERVAL_AGO, timeInterval.getPGInterval());
+            + " JOIN users ON user_id=users.id AND NOT deleted WHERE timestamp > now() - ? GROUP BY role";
+    Map<TimeInterval, Map<Role, Long>> allResults = new HashMap<>();
+
+    try (Connection conn = database.getDatabaseConnection()) {
+      for (TimeInterval timeRange : timeRanges) {
+        allResults.put(timeRange, executeQueryForTimeRange(conn, query, timeRange));
+      }
+    } catch (SQLException e) {
+      throw new SegueDatabaseException("Postgres exception", e);
+    }
+    return allResults;
+  }
 
+  private Map<Role, Long> executeQueryForTimeRange(Connection conn, String query, TimeInterval timeRange) throws SQLException {
+    Map<Role, Long> resultForTimeRange = new HashMap<>();
+    try (PreparedStatement pst = conn.prepareStatement(query)) {
+      pst.setObject(FIELD_GET_ANSWERED_QUESTION_ROLES_INTERVAL_AGO, timeRange.getPGInterval());
       try (ResultSet results = pst.executeQuery()) {
-        Map<Role, Long> resultsToReturn = Maps.newHashMap();
         while (results.next()) {
-          resultsToReturn.put(Role.valueOf(results.getString("role")), results.getLong("count"));
+          resultForTimeRange.put(Role.valueOf(results.getString("role")), results.getLong("count"));
         }
-        return resultsToReturn;
       }
-    } catch (SQLException e) {
-      throw new SegueDatabaseException("Postgres exception", e);
     }
+    return resultForTimeRange;
   }
 
+
   @Override
   public Map<Date, Long> getQuestionAttemptCountForUserByDateRange(final Date fromDate, final Date toDate,
                                                                    final Long userId, final Boolean perDay)

diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/QuestionManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/QuestionManager.java
@@ -438,13 +438,14 @@ public void mergeAnonymousQuestionAttemptsIntoRegisteredUser(final AnonymousUser
   /**
    * Count the users by role which have answered questions over the previous time interval.
    *
-   * @param timeInterval time interval over which to count
+   * @param timeIntervals An array of time ranges (in string format) for which to get the user counts.
+   *                      Each time range is used in the SQL query to filter the results.
    * @return map of counts for each role
    * @throws SegueDatabaseException - if there is a problem with the database.
    */
-  public Map<Role, Long> getAnsweredQuestionRolesOverPrevious(final TimeInterval timeInterval)
-      throws SegueDatabaseException {
-    return this.questionAttemptPersistenceManager.getAnsweredQuestionRolesOverPrevious(timeInterval);
+  public Map<TimeInterval, Map<Role, Long>> getAnsweredQuestionRolesOverPrevious(
+      final Constants.TimeInterval[] timeIntervals) throws SegueDatabaseException {
+    return this.questionAttemptPersistenceManager.getAnsweredQuestionRolesOverPrevious(timeIntervals);
   }
 
   /**

diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/StatisticsManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/StatisticsManager.java
@@ -26,11 +26,7 @@
 import static uk.ac.cam.cl.dtg.segue.api.Constants.NUMBER_DAYS_IN_LONG_MONTH;
 import static uk.ac.cam.cl.dtg.segue.api.Constants.SegueServerLogType;
 import static uk.ac.cam.cl.dtg.segue.api.Constants.TYPE_FIELDNAME;
-import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval.NINETY_DAYS;
-import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval.SEVEN_DAYS;
-import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval.SIX_MONTHS;
-import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval.THIRTY_DAYS;
-import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval.TWO_YEARS;
+import static uk.ac.cam.cl.dtg.segue.api.Constants.TimeInterval;
 import static uk.ac.cam.cl.dtg.segue.api.Constants.UNPROCESSED_SEARCH_FIELD_SUFFIX;
 
 import com.google.api.client.util.Lists;
@@ -53,6 +49,7 @@
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -152,10 +149,17 @@ public StatisticsManager(final UserAccountManager userManager, final ILogManager
    * @throws SegueDatabaseException - if there is a database error.
    */
   @Override
-  public synchronized Map<String, Object> getGeneralStatistics()
-      throws SegueDatabaseException {
-    Map<String, Object> result = Maps.newHashMap();
+  public synchronized Map<String, Object> getGeneralStatistics() throws SegueDatabaseException {
+    Map<String, Object> result = new HashMap<>();
+
+    addBasicStats(result);
+    addRangedActiveUserStats(result);
+    addRangedAnsweredQuestionStats(result);
+
+    return result;
+  }
 
+  private void addBasicStats(final Map<String, Object> result) throws SegueDatabaseException {
     result.put("userGenders", userManager.getGenderCount());
     result.put("userRoles", userManager.getRoleCount());
     result.put("userSchoolInfo", userManager.getSchoolInfoStats());
@@ -164,24 +168,29 @@ public synchronized Map<String, Object> getGeneralStatistics()
     result.put("viewQuestionEvents", logManager.getLogCountByType(IsaacServerLogType.VIEW_QUESTION.name()));
     result.put("answeredQuestionEvents", logManager.getLogCountByType(SegueServerLogType.ANSWER_QUESTION.name()));
     result.put("viewConceptEvents", logManager.getLogCountByType(IsaacServerLogType.VIEW_CONCEPT.name()));
+  }
 
-    Map<String, Map<Role, Long>> rangedActiveUserStats = Maps.newHashMap();
-    rangedActiveUserStats.put("sevenDays", userManager.getActiveRolesOverPrevious(SEVEN_DAYS));
-    rangedActiveUserStats.put("thirtyDays", userManager.getActiveRolesOverPrevious(THIRTY_DAYS));
-    rangedActiveUserStats.put("ninetyDays", userManager.getActiveRolesOverPrevious(NINETY_DAYS));
-    rangedActiveUserStats.put("sixMonths", userManager.getActiveRolesOverPrevious(SIX_MONTHS));
-    rangedActiveUserStats.put("twoYears", userManager.getActiveRolesOverPrevious(TWO_YEARS));
-    result.put("activeUsersOverPrevious", rangedActiveUserStats);
-
-    Map<String, Map<Role, Long>> rangedAnsweredQuestionStats = Maps.newHashMap();
-    rangedAnsweredQuestionStats.put("sevenDays", questionManager.getAnsweredQuestionRolesOverPrevious(SEVEN_DAYS));
-    rangedAnsweredQuestionStats.put("thirtyDays", questionManager.getAnsweredQuestionRolesOverPrevious(THIRTY_DAYS));
-    rangedAnsweredQuestionStats.put("ninetyDays", questionManager.getAnsweredQuestionRolesOverPrevious(NINETY_DAYS));
-    result.put("answeringUsersOverPrevious", rangedAnsweredQuestionStats);
+  private void addRangedActiveUserStats(final Map<String, Object> result) throws SegueDatabaseException {
+    TimeInterval[] timeIntervals = {
+        TimeInterval.SEVEN_DAYS,
+        TimeInterval.THIRTY_DAYS,
+        TimeInterval.NINETY_DAYS,
+        TimeInterval.SIX_MONTHS,
+        TimeInterval.TWO_YEARS
+    };
+    result.put("activeUsersOverPrevious", userManager.getActiveRolesOverPrevious(timeIntervals));
+  }
 
-    return result;
+  private void addRangedAnsweredQuestionStats(final Map<String, Object> result) throws SegueDatabaseException {
+    TimeInterval[] timeIntervals = {
+        TimeInterval.SEVEN_DAYS,
+        TimeInterval.THIRTY_DAYS,
+        TimeInterval.NINETY_DAYS
+    };
+    result.put("answeringUsersOverPrevious", questionManager.getAnsweredQuestionRolesOverPrevious(timeIntervals));
   }
 
+
   /**
    * LogCount.
    *

diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java
@@ -1961,12 +1961,14 @@ public Map<Role, Long> getRoleCount() throws SegueDatabaseException {
   /**
    * Count the users by role seen over the previous time interval.
    *
-   * @param timeInterval time interval over which to count
+   * @param timeIntervals An array of time ranges (in string format) for which to get the user counts.
+   *                      Each time range is used in the SQL query to filter the results.
    * @return map of counts for each role
    * @throws SegueDatabaseException - if there is a problem with the database.
    */
-  public Map<Role, Long> getActiveRolesOverPrevious(final TimeInterval timeInterval) throws SegueDatabaseException {
-    return this.database.getRolesLastSeenOver(timeInterval);
+  public Map<TimeInterval, Map<Role, Long>> getActiveRolesOverPrevious(final TimeInterval[] timeIntervals)
+      throws SegueDatabaseException {
+    return this.database.getRolesLastSeenOver(timeIntervals);
   }
 
   /**

diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/IUserDataManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/IUserDataManager.java
@@ -265,13 +265,14 @@ boolean linkAuthProviderToAccount(RegisteredUser user, AuthenticationProvider pr
   Map<Role, Long> getRoleCount() throws SegueDatabaseException;
 
   /**
-   * Count the users by role seen over the previous time interval.
+   * Retrieves a count of users by their roles who were last seen within the specified time ranges.
    *
-   * @param timeInterval time interval over which to count
-   * @return map of counts for each role
-   * @throws SegueDatabaseException - if there is a problem with the database.
+   * @param timeRanges An array of time ranges (in string format) for which to get the user counts.
+   * @return A map where the keys are the time ranges and the values are another map containing
+   *     the count of users for each role within that time range.
+   * @throws SegueDatabaseException If there is a database-related issue, such as a SQL exception.
    */
-  Map<Role, Long> getRolesLastSeenOver(TimeInterval timeInterval) throws SegueDatabaseException;
+  Map<TimeInterval, Map<Role, Long>> getRolesLastSeenOver(TimeInterval[] timeRanges) throws SegueDatabaseException;
 
   /**
    * Count users' reported genders.

diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgUsers.java b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgUsers.java
@@ -505,25 +505,40 @@ public Map<Gender, Long> getGenderCount() throws SegueDatabaseException {
   }
 
   @Override
-  public Map<Role, Long> getRolesLastSeenOver(final TimeInterval timeInterval) throws SegueDatabaseException {
-    String query = "SELECT role, count(1) FROM users WHERE NOT deleted AND last_seen >= now() - ? GROUP BY role";
-    try (Connection conn = database.getDatabaseConnection();
-         PreparedStatement pst = conn.prepareStatement(query)
-    ) {
-      pst.setObject(FIELD_GET_PERIOD_ROLES_INTERVAL, timeInterval.getPGInterval());
-
-      try (ResultSet results = pst.executeQuery()) {
-        Map<Role, Long> resultsToReturn = Maps.newHashMap();
-        while (results.next()) {
-          resultsToReturn.put(Role.valueOf(results.getString("role")), results.getLong("count"));
-        }
-        return resultsToReturn;
+  public Map<TimeInterval, Map<Role, Long>> getRolesLastSeenOver(final TimeInterval[] timeIntervals)
+      throws SegueDatabaseException {
+    Map<TimeInterval, Map<Role, Long>> allResults = new HashMap<>();
+    try (Connection conn = database.getDatabaseConnection()) {
+      for (TimeInterval timeInterval : timeIntervals) {
+        Map<Role, Long> resultForTimeInterval = executeRoleCountQueryForTimeInterval(conn, timeInterval);
+        allResults.put(timeInterval, resultForTimeInterval);
       }
     } catch (SQLException e) {
       throw new SegueDatabaseException(POSTGRES_EXCEPTION_MESSAGE, e);
     }
+    return allResults;
   }
 
+  private Map<Role, Long> executeRoleCountQueryForTimeInterval(final Connection conn, final TimeInterval timeInterval)
+      throws SQLException {
+    String query = "SELECT role, count(1) FROM users WHERE NOT deleted AND last_seen >= now() - ? GROUP BY role";
+    try (PreparedStatement pst = conn.prepareStatement(query)) {
+      pst.setObject(1, timeInterval.getPGInterval());
+      try (ResultSet results = pst.executeQuery()) {
+        return parseRoleCountsFromResults(results);
+      }
+    }
+  }
+
+  private Map<Role, Long> parseRoleCountsFromResults(final ResultSet results) throws SQLException {
+    Map<Role, Long> resultForTimeInterval = new HashMap<>();
+    while (results.next()) {
+      resultForTimeInterval.put(Role.valueOf(results.getString("role")), results.getLong("count"));
+    }
+    return resultForTimeInterval;
+  }
+
+
   @Override
   public Map<SchoolInfoStatus, Long> getSchoolInfoStats() throws SegueDatabaseException {
     String query = "SELECT school_id IS NOT NULL AS has_school_id,  school_other IS NOT NULL AS has_school_other,"