Merge pull request #212 from Arquisoft/code-smells

Fix code smells
Arquisoft · Apr 27, 2024 · 63bccef · 63bccef
2 parents 6da2da1 + 7c4ac79
commit 63bccef
Show file tree

Hide file tree

Showing 24 changed files with 98 additions and 127 deletions.
diff --git a/.github/workflows/unit-tests-push.yml b/.github/workflows/unit-tests-push.yml
@@ -74,4 +74,4 @@ jobs:
             kill $(cat spring-boot-app.pid)
       - name: Collect Jacoco report and send to Sonar
         run: |
-            ./mvnw org.jacoco:jacoco-maven-plugin:report sonar:sonar -Dsonar.projectKey=Arquisoft_wiq_es04b -Dsonar.organization=arquisoft -Dsonar.branch.name=${{ github.ref }} -Dsonar.host.url=https://sonarcloud.io -Dsonar.login=${{ secrets.SONAR_TOKEN }} -Dspring.profiles.active=test
+            ./mvnw org.jacoco:jacoco-maven-plugin:report sonar:sonar -Dsonar.projectKey=Arquisoft_wiq_es04b -Dsonar.organization=arquisoft -Dsonar.branch.name=${{ github.head_ref || github.ref_name }} -Dsonar.host.url=https://sonarcloud.io -Dsonar.login=${{ secrets.SONAR_TOKEN }} -Dspring.profiles.active=test
diff --git a/src/main/java/com/uniovi/components/generators/QuestionGeneratorV2.java b/src/main/java/com/uniovi/components/generators/QuestionGeneratorV2.java
@@ -2,12 +2,11 @@
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.uniovi.dto.AnswerDto;
-import com.uniovi.dto.CategoryDto;
-import com.uniovi.dto.QuestionDto;
 import com.uniovi.entities.Answer;
 import com.uniovi.entities.Category;
 import com.uniovi.entities.Question;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.net.URI;
@@ -22,20 +21,20 @@
 import java.util.Random;
 
 public class QuestionGeneratorV2 implements QuestionGenerator{
-
-    private JsonNode jsonNode;
-    private String language_placeholder;
-    private String question_placeholder;
-    private String answer_placeholder;
+    private final JsonNode jsonNode;
+    private final String languagePlaceholder;
+    private final String questionPlaceholder;
+    private final String answerPlaceholder;
     private String language;
 
-    private Random random = new SecureRandom();
+    private final Random random = new SecureRandom();
+    private Logger logger = LoggerFactory.getLogger(QuestionGeneratorV2.class);
 
     public QuestionGeneratorV2(JsonNode jsonNode) {
         this.jsonNode = jsonNode;
-        this.language_placeholder = jsonNode.get("language_placeholder").textValue();
-        this.question_placeholder = jsonNode.get("question_placeholder").textValue();
-        this.answer_placeholder = jsonNode.get("answer_placeholder").textValue();
+        this.languagePlaceholder = jsonNode.get("language_placeholder").textValue();
+        this.questionPlaceholder = jsonNode.get("question_placeholder").textValue();
+        this.answerPlaceholder = jsonNode.get("answer_placeholder").textValue();
     }
 
     @Override
@@ -69,9 +68,9 @@ private List<Question> generateQuestion(JsonNode question, Category cat) throws
         String answerLabel= question.get("answer").textValue();
 
         // Replace the placeholders in the query with the actual values
-        query = query.replace(language_placeholder, language).
-                replace(question_placeholder, questionLabel).
-                replace(answer_placeholder, answerLabel);
+        query = query.replace(languagePlaceholder, language).
+                replace(questionPlaceholder, questionLabel).
+                replace(answerPlaceholder, answerLabel);
 
         // Execute the query and get the results
         JsonNode results = getQueryResult(query);
@@ -91,7 +90,7 @@ private List<Question> generateQuestion(JsonNode question, Category cat) throws
 
             if (statement != null) {
                 // Generate the question statement
-                String questionStatement = statement.replace(question_placeholder, result.path(questionLabel).path("value").asText());
+                String questionStatement = statement.replace(questionPlaceholder, result.path(questionLabel).path("value").asText());
 
                 // Generate the question
                 Question q = new Question(questionStatement, options, correct, cat, language);
@@ -110,8 +109,8 @@ private List<Answer> generateOptions(JsonNode results, String correctAnswer, Str
         int tries = 0;
 
        while (options.size() < 3 && tries < 10) {
-            int random = (int) (this.random.nextFloat() * size);
-            String option = results.get(random).path(answerLabel).path("value").asText();
+            int randomIdx = random.nextInt(size);
+            String option = results.get(randomIdx).path(answerLabel).path("value").asText();
             if (!option.equals(correctAnswer) && !usedOptions.contains(option) ) {
                 usedOptions.add(option);
                 options.add(new Answer(option, false));
@@ -138,7 +137,7 @@ private String prepareStatement(JsonNode question) {
 
     private JsonNode getQueryResult(String query) throws IOException, InterruptedException {
 
-        System.out.println("Query: " + query);
+        logger.info("Query: {}", query);
         HttpClient client = HttpClient.newHttpClient();
         JsonNode resultsNode;
         String endpointUrl = "https://query.wikidata.org/sparql?query=" +

diff --git a/src/main/java/com/uniovi/configuration/SecurityConfig.java b/src/main/java/com/uniovi/configuration/SecurityConfig.java
@@ -40,7 +40,7 @@ public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
                 .csrf(csrf -> csrf
                         .ignoringRequestMatchers("/api/**")
                 )
-                .authorizeHttpRequests((authorize) ->
+                .authorizeHttpRequests(authorize ->
                         authorize
                                 .requestMatchers("/css/**", "/img/**", "/script/**").permitAll()
                                 .requestMatchers("/home/**").authenticated()

diff --git a/src/main/java/com/uniovi/controllers/CustomErrorController.java b/src/main/java/com/uniovi/controllers/CustomErrorController.java
@@ -15,6 +15,7 @@
 
 @Controller
 public class CustomErrorController extends BasicErrorController {
+    private static final String PATH = "error";
     @Autowired
     public CustomErrorController(ErrorAttributes errorAttributes, ServerProperties serverProperties, List<ErrorViewResolver> errorViewResolvers) {
         super(errorAttributes, serverProperties.getError(), errorViewResolvers);
@@ -23,10 +24,10 @@ public CustomErrorController(ErrorAttributes errorAttributes, ServerProperties s
     @RequestMapping(value = "/error")
     public String error(Model model, HttpServletRequest webRequest) {
         Map<String, Object> errorAttributes = this.getErrorAttributes(webRequest, ErrorAttributeOptions.defaults());
-        model.addAttribute("error", errorAttributes.get("error"));
+        model.addAttribute(PATH, errorAttributes.get(PATH));
         model.addAttribute("message", errorAttributes.get("message"));
         model.addAttribute("status", errorAttributes.get("status"));
         model.addAttribute("trace", errorAttributes.get("trace")); // Add the stack trace
-        return "error";
+        return PATH;
     }
 }
diff --git a/src/main/java/com/uniovi/controllers/GameController.java b/src/main/java/com/uniovi/controllers/GameController.java
@@ -24,6 +24,8 @@
 
 @Controller
 public class GameController {
+    private static final String GAMESESSION_STR = "gameSession";
+
     private final QuestionService questionService;
     private final GameSessionService gameSessionService;
     private final PlayerService playerService;
@@ -40,21 +42,21 @@ public GameController(QuestionService questionService, GameSessionService gameSe
         this.multiplayerSessionService = multiplayerSessionService;
     }
 
-
     /**
      * This method is used to get the game view and to start the game
      * @param model The model to be used
      * @return The view to be shown
      */
     @GetMapping("/game")
     public String getGame(HttpSession session, Model model, Principal principal) {
-        GameSession gameSession = (GameSession) session.getAttribute("gameSession");
+        GameSession gameSession = (GameSession) session.getAttribute(GAMESESSION_STR);
         if (gameSession != null) {
             if (checkUpdateGameSession(gameSession, session)) {
                 return "game/fragments/gameFinished";
             }
         } else {
             gameSession = gameSessionService.startNewGame(getLoggedInPlayer(principal));
+            session.setAttribute(GAMESESSION_STR, gameSession);
             playerService.deleteMultiplayerCode(gameSession.getPlayer().getId());
             session.setAttribute("gameSession", gameSession);
 
@@ -175,15 +177,15 @@ public String startMultiplayerGame( HttpSession session, Model model) {
      * shown or the timeOutFailure view is shown.
      */
     @GetMapping("/game/{idQuestion}/{idAnswer}")
-    public String getCheckResult(@PathVariable Long idQuestion, @PathVariable Long idAnswer, Model model, HttpSession session,Principal principal) {
-        GameSession gameSession = (GameSession) session.getAttribute("gameSession");
+    public String getCheckResult(@PathVariable Long idQuestion, @PathVariable Long idAnswer, Model model, HttpSession session, Principal principal) {
+        GameSession gameSession = (GameSession) session.getAttribute(GAMESESSION_STR);
         if (gameSession == null) {
             return "redirect:/game";
         }
 
         if (!gameSession.hasQuestionId(idQuestion)) {
             model.addAttribute("score", gameSession.getScore());
-            session.removeAttribute("gameSession");
+            session.removeAttribute(GAMESESSION_STR);
             return "redirect:/game"; // if someone wants to exploit the game, just redirect to the game page
         }
 
@@ -210,7 +212,7 @@ else if(questionService.checkAnswer(idQuestion, idAnswer)) {
 
     @GetMapping("/game/update")
     public String updateGame(Model model, HttpSession session, Principal principal) {
-        GameSession gameSession = (GameSession) session.getAttribute("gameSession");
+        GameSession gameSession = (GameSession) session.getAttribute(GAMESESSION_STR);
         Question nextQuestion = gameSession.getCurrentQuestion();
         if(nextQuestion == null && isMultiPlayer/*gameSession.getPlayer().getMultiplayerCode()!=null session.getAttribute("multiplayerCode") !=null*/){
             gameSessionService.endGame(gameSession);
@@ -230,7 +232,7 @@ public String updateGame(Model model, HttpSession session, Principal principal)
         }
         if (nextQuestion == null) {
             gameSessionService.endGame(gameSession);
-            session.removeAttribute("gameSession");
+            session.removeAttribute(GAMESESSION_STR);
             model.addAttribute("score", gameSession.getScore());
             return "game/fragments/gameFinished";
         }
@@ -247,10 +249,10 @@ public String updateGame(Model model, HttpSession session, Principal principal)
 
     @GetMapping("/game/finished/{points}")
     public String finishGame(@PathVariable int points, Model model, HttpSession session) {
-        GameSession gameSession = (GameSession) session.getAttribute("gameSession");
+        GameSession gameSession = (GameSession) session.getAttribute(GAMESESSION_STR);
         if (gameSession != null) {
             gameSessionService.endGame(gameSession);
-            session.removeAttribute("gameSession");
+            session.removeAttribute(GAMESESSION_STR);
         }
         model.addAttribute("score", points);
         return "game/gameFinished";
@@ -259,7 +261,7 @@ public String finishGame(@PathVariable int points, Model model, HttpSession sess
     @GetMapping("/game/points")
     @ResponseBody
     public String getPoints(HttpSession session) {
-        GameSession gameSession = (GameSession) session.getAttribute("gameSession");
+        GameSession gameSession = (GameSession) session.getAttribute(GAMESESSION_STR);
         if (gameSession != null)
             return String.valueOf(gameSession.getScore());
         else
@@ -269,7 +271,7 @@ public String getPoints(HttpSession session) {
     @GetMapping("/game/currentQuestion")
     @ResponseBody
     public String getCurrentQuestion(HttpSession session) {
-        GameSession gameSession = (GameSession) session.getAttribute("gameSession");
+        GameSession gameSession = (GameSession) session.getAttribute(GAMESESSION_STR);
         if (gameSession != null)
             return String.valueOf(gameSession.getAnsweredQuestions().size()+1);
         else
@@ -296,7 +298,7 @@ private boolean checkUpdateGameSession(GameSession gameSession, HttpSession sess
             gameSession.addAnsweredQuestion(gameSession.getCurrentQuestion());
             if (gameSession.getQuestionsToAnswer().isEmpty()) {
                 gameSessionService.endGame(gameSession);
-                session.removeAttribute("gameSession");
+                session.removeAttribute(GAMESESSION_STR);
                 return true;
             }
         }

diff --git a/src/main/java/com/uniovi/controllers/HomeController.java b/src/main/java/com/uniovi/controllers/HomeController.java
@@ -9,6 +9,8 @@
 import org.springframework.ui.Model;
 import org.springframework.web.bind.annotation.GetMapping;
 
+import java.util.Optional;
+
 @Controller
 public class HomeController{
     private final PlayerService playerService;
@@ -27,16 +29,21 @@ public String home(){
 
     @GetMapping("/home/apikey")
     public String apiKeyHome(Authentication auth, Model model) {
-        Player player = playerService.getUserByUsername(auth.getName()).get();
-        model.addAttribute("apiKey", player.getApiKey());
+        Optional<Player> playerOpt = playerService.getUserByUsername(auth.getName());
+        if (playerOpt.isPresent()) {
+            Player player = playerOpt.get();
+            model.addAttribute("apiKey", player.getApiKey());
+        }
         return "player/apiKeyHome";
     }
 
     @GetMapping("/home/apikey/create")
     public String createApiKey(Authentication auth) {
-        Player player = playerService.getUserByUsername(auth.getName()).get();
-        if (player.getApiKey() == null) {
-            apiKeyService.createApiKey(player);
+        if (playerService.getUserByUsername(auth.getName()).isPresent()) {
+            Player player = playerService.getUserByUsername(auth.getName()).get();
+            if (player.getApiKey() == null) {
+                apiKeyService.createApiKey(player);
+            }
         }
         return "redirect:/home/apikey";
     }

diff --git a/src/main/java/com/uniovi/controllers/PlayersController.java b/src/main/java/com/uniovi/controllers/PlayersController.java
@@ -20,7 +20,6 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.Pageable;
-import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Controller;
 import org.springframework.ui.Model;
 import org.springframework.validation.BindingResult;
@@ -94,7 +93,6 @@ public String showLoginForm(Model model, @RequestParam(value = "error", required
                                 HttpSession session) {
         if (error != null) {
             model.addAttribute("error", session.getAttribute("loginErrorMessage"));
-            System.out.println(session.getAttribute("loginErrorMessage"));
         }
 
         if (SecurityConfig.isAuthenticated())
@@ -247,7 +245,7 @@ public String changeRoles(HttpServletResponse response, @RequestParam String use
 
     @GetMapping("/player/admin/questionManagement")
     public String showQuestionManagementFragment(Model model) throws IOException {
-        File jsonFile = new File(QuestionGeneratorService.jsonFilePath);
+        File jsonFile = new File(QuestionGeneratorService.JSON_FILE_PATH);
         ObjectMapper objectMapper = new ObjectMapper();
         JsonNode json = objectMapper.readTree(jsonFile);
         model.addAttribute("jsonContent", json.toString());
@@ -257,7 +255,7 @@ public String showQuestionManagementFragment(Model model) throws IOException {
 
     @GetMapping("/player/admin/deleteAllQuestions")
     @ResponseBody
-    public String deleteAllQuestions() {
+    public String deleteAllQuestions() throws IOException {
         questionService.deleteAllQuestions();
         return "Questions deleted";
     }
@@ -273,7 +271,7 @@ public String saveQuestions(HttpServletResponse response, @RequestParam String j
             printer.indentArraysWith(indenter);  // Indent JSON arrays
 
             ObjectMapper mapper = new ObjectMapper();
-            mapper.writer(printer).writeValue(new FileOutputStream(QuestionGeneratorService.jsonFilePath), node);
+            mapper.writer(printer).writeValue(new FileOutputStream(QuestionGeneratorService.JSON_FILE_PATH), node);
             return "Questions saved";
         }
         catch (Exception e) {

diff --git a/src/main/java/com/uniovi/controllers/api/PlayerApiController.java b/src/main/java/com/uniovi/controllers/api/PlayerApiController.java
@@ -30,7 +30,6 @@
 import org.springframework.validation.SimpleErrors;
 import org.springframework.web.bind.annotation.*;
 
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;

diff --git a/src/main/java/com/uniovi/controllers/api/QuestionsApiController.java b/src/main/java/com/uniovi/controllers/api/QuestionsApiController.java
@@ -160,7 +160,7 @@ public String addQuestion(HttpServletResponse response, @RequestHeader("API-KEY"
             return objectMapper.writeValueAsString(error);
         }
 
-        if (questionDto.getOptions().stream().anyMatch(option -> option.isCorrect())) {
+        if (questionDto.getOptions().stream().anyMatch(AnswerDto::isCorrect)) {
             questionDto.setCorrectAnswer(questionDto.getOptions().stream().filter(option -> option.isCorrect()).findFirst().get());
         }
 

diff --git a/src/main/java/com/uniovi/entities/Associations.java b/src/main/java/com/uniovi/entities/Associations.java
@@ -3,7 +3,11 @@
 import java.util.*;
 
 public class Associations {
+    private static final String UTILITY_CLASS = "Utility class";
+    private Associations() { throw new IllegalStateException(UTILITY_CLASS); }
+
     public static class PlayerRole {
+        private PlayerRole() { throw new IllegalStateException(UTILITY_CLASS); }
         /**
          * Add a new association between a player and a role
          *
@@ -28,6 +32,7 @@ public static void removeRole(Player player, Role role) {
     }
 
     public static class PlayerApiKey {
+        private PlayerApiKey() { throw new IllegalStateException(UTILITY_CLASS); }
         /**
          * Add a new association between a player and an API key
          *
@@ -52,6 +57,7 @@ public static void removeApiKey(Player player, ApiKey apiKey) {
     }
 
     public static class ApiKeyAccessLog {
+        private ApiKeyAccessLog() { throw new IllegalStateException(UTILITY_CLASS); }
         /**
          * Add a new association between an API key and an access log
          *
@@ -76,6 +82,7 @@ public static void removeAccessLog(ApiKey apiKey, RestApiAccessLog accessLog) {
     }
 
     public static class PlayerGameSession {
+        private PlayerGameSession() { throw new IllegalStateException(UTILITY_CLASS); }
         /**
          * Add a new association between a player and a game session
          *
@@ -101,6 +108,7 @@ public static void removeGameSession(Player player, GameSession gameSession) {
     }
 
     public static class QuestionAnswers {
+        private QuestionAnswers() { throw new IllegalStateException(UTILITY_CLASS); }
         /**
          * Add a new association between a question and an answer
          *
@@ -130,15 +138,10 @@ public static void removeAnswer(Question question, List<Answer> answer) {
             }
             question.setCorrectAnswer(null);
         }
-        //public static void removeAnswer(Question question, List<Answer> answer) {
-          //  question.getOptions().remove(answer);
-            //for (Answer a : answer) {
-              //  a.setQuestion(null);
-            //}
-        //}
     }
 
     public static class QuestionsCategory {
+        private QuestionsCategory() { throw new IllegalStateException(UTILITY_CLASS); }
         /**
          * Add a new association between a question and a category
          *

diff --git a/src/main/java/com/uniovi/entities/GameSession.java b/src/main/java/com/uniovi/entities/GameSession.java
@@ -4,7 +4,6 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.uniovi.interfaces.JsonEntity;
 import jakarta.persistence.*;
-import jakarta.validation.constraints.NotEmpty;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.Setter;