13
\$\begingroup\$

I have to write a simple web application using the Java Spring framework as my course project. So I decided to write a simple Stack Overflow clone. My application has the following features:

  • Authorization (using Spring Security);
  • Posting new questions and answers;
  • Voting up/down for questions/answers.

I am very new in Java, Spring, and web-backend world so I think there is much room for improvement.


Some of my thoughts about problems in my Java code:

  • The VotesController class consists of several almost identical methods. I know that copy-paste is bad, but I have no idea how to deal with it in this case.

  • I am not sure about naming conventions in Spring. Have I properly named controllers, entities, fields, etc.?

  • I really hate the way I pass information to the Mustache templates. For example, I need to display a question's creation date in this form: May 27 '20 at 15:40, but if I just use Date creationDateTime field from the Question entity Mustache will display it in form 2020-05-27 15:40:49.0.

    To solve this problem I have created the String formattedCreationDateTime field in the Question entity and call the Question.formatCreationDateTime method just before passing the question entity to Mustache.

    And then I can use formattedCreationDateTime in the template. It is not the single one example.

  • I also do not like the way I store votes for questions/answers. At this time I have four different join tables:

    question_vote_up(question_id, user_id)
    question_vote_down(question_id, user_id)
    answer_vote_up(answer_id, user_id)
    answer_vote_down(answer_id, user_id)
    

    I know that it would be better to create only two tables like this:

    question_vote(question_id, user_id, vote)
    answer_vote(answer_id, user_id, vote)
    

    But I don't know how to implement this database structure in Spring.


I would really appreciate any advice on how to improve my code. I would be glad to see review on my JavaScript and CSS, but it is not a priority.

I've published all the code in the GitHub repository.


Controllers

addAnswerController.java:

package com.sstu.StackCanary.controllers;

import java.util.*;

import com.sstu.StackCanary.domain.Answer;
import com.sstu.StackCanary.domain.Question;
import com.sstu.StackCanary.domain.User;
import com.sstu.StackCanary.repositories.AnswerRepository;
import com.sstu.StackCanary.repositories.QuestionRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestParam;

@Controller
public class AddAnswerController {
    @Autowired
    private QuestionRepository questionRepository;

    @Autowired
    private AnswerRepository answerRepository;

    @PostMapping("/q")
    public String postQuestion(@AuthenticationPrincipal User user,
                               @RequestParam Integer questionId,
                               @RequestParam String body,
                               Map<String, Object> model) {
        // Assuming that the question with given ID always exists.
        Question q = questionRepository.findById(questionId).get();

        // Add new answer to the database.
        answerRepository.save(new Answer(user, q, body));

        // Redirect to the question page.
        return "redirect:/q?id=" + questionId;
    }
}

AskQuestionPageController:

package com.sstu.StackCanary.controllers;

import java.util.*;

import com.sstu.StackCanary.domain.Question;
import com.sstu.StackCanary.domain.Tag;
import com.sstu.StackCanary.domain.User;
import com.sstu.StackCanary.repositories.QuestionRepository;
import com.sstu.StackCanary.repositories.TagRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestParam;

@Controller
public class AskQuestionPageController {
    @Autowired
    private QuestionRepository questionRepository;

    @Autowired
    private TagRepository tagRepository;

    @GetMapping("/askQuestion")
    public String main(@AuthenticationPrincipal User user,
                       Map<String, Object> model) {
        model.put("authorizedUser", user);
        return "askQuestion";
    }

    @PostMapping("/askQuestion")
    public String postQuestion(@AuthenticationPrincipal User user,
                               @RequestParam String title,
                               @RequestParam String body,
                               @RequestParam("tag") String [] tagNames,
                               Map<String, Object> model) {
        // Create empty set of tags.
        HashSet<Tag> tags = new HashSet<Tag>();

        // Fill this set with tags with given name from database.
        // If the tag not exist create such new one.
        for (String name : tagNames) {
            Tag tag = tagRepository.findByName(name);

            if (tag == null)
                tag = new Tag(name);

            tagRepository.save(tag);
            tags.add(tag);
        }

        // Create new question and save it in the database.
        Question q = new Question(user, title, body, tags);
        questionRepository.save(q);

        // Redirect to the new question's page.
        return "redirect:/q?id=" + q.getId();
    }
}

IndexController.java:

package com.sstu.StackCanary.controllers;

import java.util.Map;

import com.sstu.StackCanary.domain.Question;
import com.sstu.StackCanary.domain.User;
import com.sstu.StackCanary.repositories.QuestionRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;

@Controller
public class IndexController {
    @Autowired
    private QuestionRepository questionRepository;

    @GetMapping
    public String main(@AuthenticationPrincipal User user,
                       Map<String, Object> model) {
        Iterable<Question> questions = questionRepository.findAll();

        // Prepare transient fields
        //
        // — formattedCreationDateTime
        // — votes
        //
        // that will be used in the template.
        questions.forEach(Question::calculateVotes);
        questions.forEach(Question::formatCreationDateTime);

        model.put("questions", questions);
        model.put("authorized", (user != null));
        return "index";
    }
}

QuestionPageController:

package com.sstu.StackCanary.controllers;

import java.util.Map;

import com.sstu.StackCanary.domain.Answer;
import com.sstu.StackCanary.domain.Question;
import com.sstu.StackCanary.domain.User;
import com.sstu.StackCanary.repositories.QuestionRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;

@Controller
public class QuestionPageController {
    @Autowired
    private QuestionRepository questionRepository;

    @GetMapping("/q")
    public String main(@AuthenticationPrincipal User user,
                       @RequestParam Integer id,
                       Map<String, Object> model) {
        // Assuming that the question with
        // given ID always exists.
        Question q = questionRepository.findById(id).get();

        // Prepare transient fields
        //
        // — formattedCreationDateTime
        // — votes
        // — answersCount
        // — bodyInHTML
        //
        // that will be used in the template.
        q.calculateVotes();
        q.calculateAnswersCount();
        q.formatCreationDateTime();
        q.convertBodyFromMarkdownToHTML();
        q.setVotedByActiveUser(user);

        // Prepare transient fields of the each answer as well
        // as we have done with the question.
        q.answers.forEach(Answer::formatCreationDateTime);
        q.answers.forEach(Answer::calculateVotes);
        q.answers.forEach(Answer::convertBodyFromMarkdownToHTML);
        q.answers.forEach(a -> a.setVotedByActiveUser(user));

        model.put("question", q);
        model.put("authorized", (user != null));

        return "question";
    }
}

RegistrationController:

package com.sstu.StackCanary.controllers;

import com.sstu.StackCanary.domain.Role;
import com.sstu.StackCanary.domain.User;
import com.sstu.StackCanary.repositories.UserRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;

import java.util.Collections;
import java.util.Map;

@Controller
public class RegistrationController {
    @Autowired
    private UserRepository userRepository;

    @GetMapping("/registration")
    public String main(Map<String, Object> model) {
        return "registration";
    }

    @PostMapping("/registration")
    public String registerUser(User user, Map<String, Object> model) {
        if (userWithThisUsernameAlreadyExists(user)) {
            model.put("userWithThisUsernameAlreadyExistsMessage", "User with this username already exists.");
            return "registration";
        }

        user.setActive(true);
        user.setRoles(Collections.singleton(Role.USER));
        userRepository.save(user);

        return "redirect:/login";
    }

    private boolean userWithThisUsernameAlreadyExists(User u) {
        return userRepository.findByUsername(u.getUsername()) != null;
    }
}

VotesController:

package com.sstu.StackCanary.controllers;

import com.sstu.StackCanary.domain.Answer;
import com.sstu.StackCanary.domain.Question;
import com.sstu.StackCanary.domain.User;
import com.sstu.StackCanary.repositories.AnswerRepository;
import com.sstu.StackCanary.repositories.QuestionRepository;
import com.sstu.StackCanary.repositories.UserRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestParam;

import java.util.Map;

@Controller
public class VotesController {
    @Autowired
    private AnswerRepository answerRepository;

    @Autowired
    private QuestionRepository questionRepository;

    @Autowired
    private UserRepository userRepository;

    @PostMapping("/voteUpForAnswer")
    public String voteUpForAnswer(@AuthenticationPrincipal User user,
                                  @RequestParam Integer questionId,
                                  @RequestParam Integer answerId,
                                  Map<String, Object> model) {
        Answer answer = answerRepository.findById(answerId).get();

        answer.votedUpByUsers.add(user);
        answer.votedDownByUsers.remove(user);

        user.voteUpForAnswer(answer);

        answerRepository.save(answer);
        userRepository.save(user);

        return "redirect:/q?id=" + questionId;
    }

    @PostMapping("/undoVoteUpForAnswer")
    public String undoVoteUpForAnswer(@AuthenticationPrincipal User user,
                                      @RequestParam Integer answerId,
                                      Map<String, Object> model) {
        Answer answer = answerRepository.findById(answerId).get();

        answer.votedUpByUsers.remove(user);
        user.getVotedUpAnswers().remove(answer);

        answerRepository.save(answer);
        userRepository.save(user);

        return "redirect:/q?id=" + answerId;
    }

    @PostMapping("/voteDownForAnswer")
    public String voteDownForAnswer(@AuthenticationPrincipal User user,
                                    @RequestParam Integer questionId,
                                    @RequestParam Integer answerId,
                                    Map<String, Object> model) {
        Answer answer = answerRepository.findById(answerId).get();

        answer.votedDownByUsers.add(user);
        answer.votedUpByUsers.remove(user);

        user.voteDownForAnswer(answer);

        answerRepository.save(answer);
        userRepository.save(user);

        return "redirect:/q?id=" + questionId;
    }

    @PostMapping("/undoVoteDownForAnswer")
    public String undoVoteDownForAnswer(@AuthenticationPrincipal User user,
                                        @RequestParam Integer answerId,
                                        Map<String, Object> model) {
        Answer answer = answerRepository.findById(answerId).get();

        answer.votedDownByUsers.remove(user);
        user.getVotedDownAnswers().remove(answer);

        answerRepository.save(answer);
        userRepository.save(user);

        return "redirect:/q?id=" + answerId;
    }

    @PostMapping("/voteUpForQuestion")
    public String voteUpForQuestion(@AuthenticationPrincipal User user,
                                    @RequestParam Integer questionId,
                                    Map<String, Object> model) {
        Question question = questionRepository.findById(questionId).get();

        question.votedUpByUsers.add(user);
        question.votedDownByUsers.remove(user);

        user.voteUpForQuestion(question);

        questionRepository.save(question);
        userRepository.save(user);

        return "redirect:/q?id=" + questionId;
    }

    @PostMapping("/undoVoteUpForQuestion")
    public String undoVoteUpForQuestion(@AuthenticationPrincipal User user,
                                        @RequestParam Integer questionId,
                                        Map<String, Object> model) {
        Question question = questionRepository.findById(questionId).get();

        question.votedUpByUsers.remove(user);
        user.getVotedUpQuestions().remove(question);

        questionRepository.save(question);
        userRepository.save(user);

        return "redirect:/q?id=" + questionId;
    }

    @PostMapping("/voteDownForQuestion")
    public String voteDownForQuestion(@AuthenticationPrincipal User user,
                                      @RequestParam Integer questionId,
                                      Map<String, Object> model) {
        Question question = questionRepository.findById(questionId).get();

        question.votedDownByUsers.add(user);
        question.votedUpByUsers.remove(user);

        user.voteDownForQuestion(question);

        questionRepository.save(question);
        userRepository.save(user);

        return "redirect:/q?id=" + questionId;
    }

    @PostMapping("/undoVoteDownForQuestion")
    public String undoVoteDownForQuestion(@AuthenticationPrincipal User user,
                                          @RequestParam Integer questionId,
                                          Map<String, Object> model) {
        Question question = questionRepository.findById(questionId).get();

        question.votedDownByUsers.remove(user);
        user.getVotedDownQuestions().remove(question);

        questionRepository.save(question);
        userRepository.save(user);

        return "redirect:/q?id=" + questionId;
    }
}

Entities

Answer.java:

package com.sstu.StackCanary.domain;

import org.commonmark.node.Node;
import org.commonmark.parser.Parser;
import org.commonmark.renderer.html.HtmlRenderer;

import javax.persistence.*;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.Set;

@Entity
public class Answer {
    //==========================================
    //
    // Database Columns
    //
    //==========================================

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Integer id;

    @Column(columnDefinition = "LONGTEXT")
    private String body;

    @Column(name = "creationDateTime", columnDefinition = "DATETIME")
    @Temporal(TemporalType.TIMESTAMP)
    private Date creationDateTime;

    //==========================================
    //
    // Relations
    //
    //==========================================

    @ManyToOne(fetch = FetchType.EAGER)
    @JoinColumn(name = "author")
    private User author;

    @ManyToOne
    @JoinColumn(name = "question", nullable = false)
    private Question question;

    @ManyToMany
    @JoinTable(
            name = "answer_vote_up",
            joinColumns = @JoinColumn(name = "answer_id"),
            inverseJoinColumns = @JoinColumn(name = "user_id")
    )
    public Set<User> votedUpByUsers;

    @ManyToMany
    @JoinTable(
            name = "answer_vote_down",
            joinColumns = @JoinColumn(name = "answer_id"),
            inverseJoinColumns = @JoinColumn(name = "user_id")
    )
    public Set<User> votedDownByUsers;

    //==========================================
    //
    // Transient Fields
    //
    // This fields must be initialized manually by
    // calling the corresponding entity's method.
    //==========================================

    @Transient
    private String formattedCreationDateTime;

    @Transient
    public Integer votes;

    @Transient
    public String bodyInHTML;

    @Transient
    public boolean votedUpByActiveUser;

    @Transient
    public boolean votedDownByActiveUser;

    //==========================================
    //
    // Constructors
    //
    //==========================================

    protected Answer() {}

    public Answer(User author, Question question, String body) {
        this.author = author;
        this.question = question;
        this.body = body;

        // Assign current date and time.
        this.creationDateTime = new Date();
    }

    //==========================================
    //
    // Methods
    //
    //==========================================

    public void formatCreationDateTime() {
        DateFormat d = new SimpleDateFormat("MMM d ''yy 'at' HH:mm");
        formattedCreationDateTime = d.format(creationDateTime);
    }

    public void calculateVotes() {
        votes = votedUpByUsers.size() - votedDownByUsers.size();
    }

    public void convertBodyFromMarkdownToHTML() {
        Node           document  =  Parser.builder().build().parse(body);
        HtmlRenderer   renderer  =  HtmlRenderer.builder().escapeHtml(true).build();
        bodyInHTML               =  renderer.render(document);
    }

    public void setVotedByActiveUser(User user) {
        if (user == null) {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = false;
        } else if (user.getVotedUpAnswers().contains(this)) {
            this.votedUpByActiveUser = true;
            this.votedDownByActiveUser = false;
        } else if (user.getVotedDownAnswers().contains(this)) {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = true;
        } else {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = false;
        }
    }

    @Override
    public boolean equals(Object that) {
        if (this == that)
            return true;

        if (!(that instanceof Answer))
            return false;

        Answer thatAnswer = (Answer) that;

        return this.id.equals(thatAnswer.id);
    }

    @Override
    public int hashCode() {
        final int PRIME = 37;
        return PRIME * id.hashCode();
    }
}

Question.java:

package com.sstu.StackCanary.domain;

import org.commonmark.node.Node;
import org.commonmark.parser.Parser;
import org.commonmark.renderer.html.HtmlRenderer;

import javax.persistence.*;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.Set;

@Entity
public class Question {
    //==========================================
    //
    // Database Columns
    //
    //==========================================

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Integer id;

    private String title;

    @Column(columnDefinition = "LONGTEXT")
    private String body;

    @Column(name = "creationDateTime", columnDefinition = "DATETIME")
    @Temporal(TemporalType.TIMESTAMP)
    private Date creationDateTime;

    //==========================================
    //
    // Relations
    //
    //==========================================

    @ManyToOne(fetch = FetchType.EAGER)
    @JoinColumn(name = "author")
    private User author;

    @ManyToMany(fetch = FetchType.EAGER)
    @JoinTable(
        name = "question_tag",
        joinColumns = @JoinColumn(name = "question_id"),
        inverseJoinColumns = @JoinColumn(name = "tag_id")
    )
    private Set<Tag> tags;

    @OneToMany(mappedBy = "question", fetch = FetchType.EAGER)
    public Set<Answer> answers;

    @ManyToMany(fetch = FetchType.EAGER)
    @JoinTable(
            name = "question_vote_up",
            joinColumns = @JoinColumn(name = "question_id"),
            inverseJoinColumns = @JoinColumn(name = "user_id")
    )
    public Set<User> votedUpByUsers;

    @ManyToMany(fetch = FetchType.EAGER)
    @JoinTable(
            name = "question_vote_down",
            joinColumns = @JoinColumn(name = "question_id"),
            inverseJoinColumns = @JoinColumn(name = "user_id")
    )
    public Set<User> votedDownByUsers;

    //==========================================
    //
    // Transient Fields
    //
    // This fields must be initialized manually by
    // calling the corresponding entity's method.
    //==========================================

    @Transient
    public String formattedCreationDateTime;

    @Transient
    public Integer votes;

    @Transient
    public Integer answersCount;

    @Transient
    public String bodyInHTML;

    @Transient
    public boolean votedUpByActiveUser;

    @Transient
    public boolean votedDownByActiveUser;

    //==========================================
    //
    // Constructors
    //
    //==========================================

    protected Question() {}

    public Question(User author, String title, String body, Set<Tag> tags) {
        this.author = author;
        this.title = title;
        this.body = body;
        this.tags = tags;

        // Assign current date and time.
        this.creationDateTime = new Date();
    }

    //==========================================
    //
    // Getters and Setters
    //
    //==========================================

    public Integer getId() {
        return id;
    }

    //==========================================
    //
    // Methods
    //
    //==========================================

    public void formatCreationDateTime() {
        DateFormat d = new SimpleDateFormat("MMM d ''yy 'at' HH:mm");
        formattedCreationDateTime = d.format(creationDateTime);
    }

    public void calculateVotes() {
        votes = votedUpByUsers.size() - votedDownByUsers.size();
    }

    public void calculateAnswersCount() {
        answersCount = this.answers.size();
    }

    public void convertBodyFromMarkdownToHTML() {
        Node           document  =  Parser.builder().build().parse(body);
        HtmlRenderer   renderer  =  HtmlRenderer.builder().escapeHtml(true).build();
        bodyInHTML               =  renderer.render(document);
    }

    public void setVotedByActiveUser(User user) {
        if (user == null) {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = false;
        } else if (user.getVotedUpQuestions().contains(this)) {
            this.votedUpByActiveUser = true;
            this.votedDownByActiveUser = false;
        } else if (user.getVotedDownQuestions().contains(this)) {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = true;
        } else {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = false;
        }
    }

    @Override
    public boolean equals(Object that) {
        if (this == that)
            return true;

        if (!(that instanceof Question))
            return false;

        Question thatQuestion = (Question) that;

        return this.id.equals(thatQuestion.id);
    }

    @Override
    public int hashCode() {
        final int PRIME = 37;
        return PRIME * id.hashCode();
    }
}

User.java:

package com.sstu.StackCanary.domain;

import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.userdetails.UserDetails;

import javax.persistence.*;
import java.util.Collection;
import java.util.Set;

@Entity
public class User implements UserDetails {
    //==========================================
    //
    // Database Columns
    //
    //==========================================

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Integer id;
    private String username;
    private String password;
    private Boolean active;

    @ElementCollection(targetClass = Role.class, fetch = FetchType.EAGER)
    @CollectionTable(name = "user_role", joinColumns = @JoinColumn(name = "user_id"))
    @Enumerated(EnumType.STRING)
    @Column(name = "role")
    private Set<Role> roles;

    //==========================================
    //
    // Relations
    //
    //==========================================

    @ManyToMany(mappedBy = "votedUpByUsers", fetch = FetchType.EAGER)
    private Set<Question> votedUpQuestions;

    @ManyToMany(mappedBy = "votedDownByUsers", fetch = FetchType.EAGER)
    private Set<Question> votedDownQuestions;

    @ManyToMany(mappedBy = "votedUpByUsers", fetch = FetchType.EAGER)
    private Set<Answer> votedUpAnswers;

    @ManyToMany(mappedBy = "votedDownByUsers", fetch = FetchType.EAGER)
    private Set<Answer> votedDownAnswers;

    //==========================================
    //
    // Constructors
    //
    //==========================================

    protected User() {}

    //==========================================
    //
    // Getters and Setters
    //
    //==========================================

    public String getUsername() {
        return username;
    }

    public void setUsername(String username) {
        this.username = username;
    }

    public Boolean getActive() {
        return active;
    }

    public void setActive(Boolean active) {
        this.active = active;
    }

    public Set<Role> getRoles() {
        return roles;
    }

    public void setRoles(Set<Role> roles) {
        this.roles = roles;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    public Integer getId() {
        return id;
    }

    public void setId(Integer id) {
        this.id = id;
    }

    public Set<Question> getVotedUpQuestions() {
        return votedUpQuestions;
    }

    public void setVotedUpQuestions(Set<Question> votedUpQuestions) {
        this.votedUpQuestions = votedUpQuestions;
    }

    public Set<Question> getVotedDownQuestions() {
        return votedDownQuestions;
    }

    public void setVotedDownQuestions(Set<Question> votedDownQuestions) {
        this.votedDownQuestions = votedDownQuestions;
    }

    public Set<Answer> getVotedUpAnswers() {
        return votedUpAnswers;
    }

    public void setVotedUpAnswers(Set<Answer> votedUpAnswers) {
        this.votedUpAnswers = votedUpAnswers;
    }

    public Set<Answer> getVotedDownAnswers() {
        return votedDownAnswers;
    }

    public void setVotedDownAnswers(Set<Answer> votedDownAnswers) {
        this.votedDownAnswers = votedDownAnswers;
    }

    @Override
    public boolean equals(Object that) {
        if (this == that)
            return true;

        if (!(that instanceof User))
            return false;

        User thatUser = (User) that;

        return this.id.equals(thatUser.id);
    }

    @Override
    public int hashCode() {
        final int PRIME = 37;
        return PRIME * id.hashCode();
    }

    public void voteUpForQuestion(Question q) {
        votedUpQuestions.add(q);
        votedDownQuestions.remove(q);
    }

    public void voteDownForQuestion(Question q) {
        votedDownQuestions.add(q);
        votedUpQuestions.remove(q);
    }

    public void voteUpForAnswer(Answer q) {
        votedUpAnswers.add(q);
        votedDownAnswers.remove(q);
    }

    public void voteDownForAnswer(Answer q) {
        votedDownAnswers.add(q);
        votedUpAnswers.remove(q);
    }

    //==========================================
    //
    // UserDetails abstract methods implementation
    //
    //==========================================

    @Override
    public Collection<? extends GrantedAuthority> getAuthorities() {
        return getRoles();
    }

    @Override
    public boolean isAccountNonExpired() {
        return true;
    }

    @Override
    public boolean isAccountNonLocked() {
        return true;
    }

    @Override
    public boolean isCredentialsNonExpired() {
        return true;
    }

    @Override
    public boolean isEnabled() {
        return getActive();
    }
}

JS scripts:

questionAndAnswersBodyRendering.js:

"use strict";

function renderQuestionAndAnswersBodies() {
    convertQuestionBodyToHTML();
    convertAnswersBodiesToHTML();
    highlightCodeInQuestion();
    highlightCodeInAnswers();
}

function convertQuestionBodyToHTML() {
    let questionBody = document.getElementById("questionBody");
    questionBody.innerHTML = replaceHTMLEntitiesWithRealCharacters(questionBody.innerHTML);

    // Add support for HTML tags inside Markdown code
    // that comes from the server.
    for (let e of questionBody.getElementsByTagName("*"))
        if (e.tagName !== "CODE" && e.tagName !== "PRE")
            e.innerHTML = replaceHTMLEntitiesWithRealCharacters(e.innerHTML);
}

function convertAnswersBodiesToHTML() {
    let answersBodies = document.getElementsByClassName("answerBody");

    for (let a of answersBodies) {
        a.innerHTML = replaceHTMLEntitiesWithRealCharacters(a.innerHTML);

        // Add support for HTML tags inside Markdown code
        // that comes from the server.
        for (let e of a.getElementsByTagName("*"))
            if (e.tagName !== "CODE")
                e.innerHTML = replaceHTMLEntitiesWithRealCharacters(e.innerHTML);
    }
}

function replaceHTMLEntitiesWithRealCharacters(string) {
    function replaceAll(string, search, replace) {
      return string.split(search).join(replace);
    }

    string = replaceAll(string,  "&lt;", "<");
    string = replaceAll(string,  "&gt;", ">");

    // This HTML entity should be the last since
    // it can affect on the other entities.
    string = replaceAll(string, "&amp;", "&");

    return string;
}

function highlightCodeInQuestion() {
    let questionBody = document.getElementById("questionBody");
    highlightCodeInsideElement(questionBody);
}

function highlightCodeInAnswers() {
    let answersBodies = document.getElementsByClassName("answerBody");

    for (let a of answersBodies)
        highlightCodeInsideElement(a);
}

function highlightCodeInsideElement(element) {
    let children = element.getElementsByTagName("*");

    for (let c of children)
        if (c.tagName === "CODE" && c.parentElement.tagName === "PRE")
            hljs.highlightBlock(c);
}

questionEditor.js:

"use strict";

let tagsList = [];
const MAX_TAGS_COUNT = 5;

function tagEditorInputOnInput() {
    var tagEditorInput = document.getElementById("tagEditorInput");

    function clearInput() {
        tagEditorInput.value = "";
    }

    let   value            = tagEditorInput.value;
    let   length           = value.length;
    const firstCharacter   = getStringFirstCharacter(value);
    const lastCharacter    = getStringLastCharacter(value);

    if (tagsList.length >= MAX_TAGS_COUNT) {
        clearInput();
    } else if (length < 2 && firstCharacter === " ") {
        clearInput();
    } else if (lastCharacter === " ") {
        const tagName = value.toLowerCase().trim();
        tagsList.push(tagName);
        clearInput();
        renderTags();
        updateTagInputs();
    }
}

function renderTags() {
    removeAllRenderedTags();

    let renderedTags = document.getElementById("renderedTags");

    for (let t of tagsList)
        renderedTags.appendChild(createRendererTagElement(t));
}

function createRendererTagElement(tagName) {
    let tag = document.createElement("span");
    addClass(tag, "renderedTag");

    tag.innerHTML  = '<span class="tagName">' + tagName + '</span>';
    tag.innerHTML += '<svg onmouseup="removeRenderedTag(this.parentElement.firstChild);" class="removeTagButton" width="14" height="14" viewBox="0 0 14 14"><path d="M12 3.41L10.59 2 7 5.59 3.41 2 2 3.41 5.59 7 2 10.59 3.41 12 7 8.41 10.59 12 12 10.59 8.41 7z"></path></svg>';

    return tag;
}

function removeAllRenderedTags() {
    let renderedTags = document.getElementById("renderedTags");
    renderedTags.innerHTML = "";
}

function removeRenderedTag(element) {
    const tagName  = getFirstWordInString(element.innerHTML);
    const tagIndex = tagsList.indexOf(tagName);

    removeItemFromArray(tagsList, tagIndex);
    renderTags();
}

function updateTagInputs() {
    for (let i = 0; i < 5; ++i) {
        let tag = document.getElementById("tag" + i);

        if (tagsList[i] === undefined)
            tag.name = "emptyTag";
        else
            tag.name = "tag";

        tag.value = tagsList[i];
    }
}

function removeLastCharacterInString(s) {
    return s.substring(0, s.length - 1);
}

function getStringLastCharacter(s) {
    return s.slice(s.length - 1);
}

function getStringFirstCharacter(s) {
    return s[0];
}

function getFirstWordInString(s) {
    const spaceIndex = s.indexOf(" ");

    if (spaceIndex === -1)
        return s;
    else
        return s.substr(0, spaceIndex);
};

function removeItemFromArray(array, index) {
    array.splice(index, 1);
}

function addClass(element, className) {
    element.classList.add(className);
}

function removeClass(element, className) {
    if (element.classList.contains(className))
        element.classList.remove(className);
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Dang, now that's a lot of files to review. \$\endgroup\$
    – xilpex
    Jun 1, 2020 at 19:35
  • \$\begingroup\$ @eanmos, i added more feedback. take a look \$\endgroup\$
    – gtiwari333
    Jun 2, 2020 at 23:49

2 Answers 2

7
+300
\$\begingroup\$

General Feedback

     HashSet<Tag> tags = new HashSet<Tag>(); // not so good
     HashSet<Tag> tags = new HashSet<>(); //this is better
     Set<Tag> tags = new HashSet<>(); //this is even better
  • Answer and Question can be combined to a single Entity (let's name it POST) and provide a Type column ( can be Enum) to indicate the type of post. It would remove lots of duplications in your code.
    Post {

      Type type;
      List<Post> answers; //only post of type 'Question' can have answers

    }
  • User entity is doing too much. Move votedUpQuestions, votedDownQuestions, votedUpAnswers, votedDownAnswers to a new table. Name it Vote with post_id and user_id as composite key. Just add a new record
    Vote{
        Post post;
        User user;
        VoteType type; //can be enum so that you can add thumbs up, like, haha etc easily later
    }
  • IndexController.main() Use separate query to find votes. You can use caching for performance. Similar feedback on QuestionPageController

  • Question.convertBodyFromMarkdownToHTML Its better to keep the entity classes as POJO.

  • When fetching a large entity and child object, use JOIN-Fetch or @EntityGraph. Lookup N+1 problem in ORM to know more about it

  • application.properties Use in-memory db like H2 so its easy to test your app. You can use Spring's @Profiles to use H2 locally and mariadb in some other profile.

  • resources/static/** You can use Webjar instead of copying javascript/css etc manually


Regarding your questions,

The VotesController class consists of several almost identical methods. I know that copy-past is bad, but I have no idea how to deal with it in this case.

  • You can create a VoteService and have a single method vote(User, Post, Type). You can keep your methods on controller but compose the Type parameter and delegate the call to service. If you merge Answer and Question table, lots of duplications can be avoided.

I am not sure about naming conventions in Spring. Have I properly named controllers, entities, fields, etc?

I really hate the way I pass information to the Mustache templates.

Finally, I noticed you are using JDK8. Is there any reason to must use JDK8? Your code runs fine with JDK11 without any update. You should try that.

Also, you can check the following repos for a reference - https://github.com/gtiwari333/spring-boot-web-application-seed

--

Update 1

Lombok's version is managed by Spring. You don't need to provide version here. See this for the list - https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-dependencies/build.gradle - https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/2.3.0.RELEASE/spring-boot-dependencies-2.3.0.RELEASE.pom

That way you can get rid of compatibility between various libraries because Spring Boot takes care of that for you.

  • AddAnswerController and other places

Since you have Lombok in place, you can replace the following code by Constructor injection using @RequiredArgsConstructor

@Controller
public class AddAnswerController {
    @Autowired
    private QuestionRepository questionRepository;

    @Autowired
    private AnswerRepository answerRepository; 


@Controller
@RequiredArgsConstructor
public class AddAnswerController {    
    private final QuestionRepository questionRepository;
    private final AnswerRepository answerRepository; 
..
  • Don't fetch all voted*ByUsers records just to find the size. Imagine billions of Vote records per Question. You are currently fetching billions of DB records and doing size() operation just to get the count. It would be a single COUNT query on DB. Remember always delegate count, exists, search operation to DB.
    public void calculateVotes() {
        votes = votedUpByUsers.size() - votedDownByUsers.size();
    }

    //here we are fetching all answers from DB to memory just to get the size. 
    public void calculateAnswersCount() {
        answersCount = this.answers.size();
    }

    public void setVotedByActiveUser(User user) {
        if (user == null) {
            this.votedUpByActiveUser = false;
            this.votedDownByActiveUser = false;
        } else if (user.getVotedUpQuestions().contains(this)) { //FIX THIS
            this.votedUpByActiveUser = true;
            this.votedDownByActiveUser = false;
        } else if (user.getVotedDownQuestions().contains(this)) { //FIX THIS
    ...
    }


  • You can move this to a util/service class so that it will be reusable and easily unit tested
    public void convertBodyFromMarkdownToHTML() {
        Node           document  =  Parser.builder().build().parse(body);
        HtmlRenderer   renderer  =  HtmlRenderer.builder().escapeHtml(true).build();
        bodyInHTML               =  renderer.render(document);
    }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I very appreciate your answer. You helped me a lot. Thank you! \$\endgroup\$
    – eanmos
    Jun 3, 2020 at 10:03
1
\$\begingroup\$

I haven't done much Java development since I was a univeristy student 15 years ago so my assistance there will be limited. The bulk of this review will be on the JavaScript code.

Java

Comments

There are single line comments used for multiple lines of text - Doc comments or Block comments could be used instead to follow common conventions.

For example, instead of :

// Prepare transient fields
//
// — formattedCreationDateTime
// — votes
//
// that will be used in the template.

Use a block comment:

/* 
* Prepare transient fields
*
* — formattedCreationDateTime
* — votes
*
* that will be used in the template.
*/

And also:

//==========================================
//
// Database Columns
//
//==========================================

Use a block comment:

/*
* ==========================================
*
* Database Columns
*
* ==========================================
*/

Braces

It is better to use braces around control structures even if they contain a single statement. If you or a colleague adds a statement intending to add a block then missing braces could lead to logical errors.

Exception/Error handling

While it may be a rare scenario, what would happen if a question or answer wasn't found when a user attempts to vote - e.g. if deleting is possible.

JavaScript

There are many things I spot. Using a linter like JSLint, esLint, etc. would find many of these things.

Braces

As mentioned above for Java, braces aren't required for control structures but it helps avoid bugs when you or a colleague need to add lines to blocks within those control structures.

Semicolons

Semicolons aren't required for all lines except a handful of statements so as this blog post explains it is best to use them to avoid unintentional behavior in your code.

Varible scope, initilization

Some variables are declared using let - e.g. questionBody in convertQuestionBodyToHTML(), but these are never re-assigned. It is best to default to using const and then when it is determined that re-assignment is necessary use let. This even applies to arrays when elements are only pushed into them.

There is the variable tagEditorInput in tagEditorInputOnInput() declared with var. This should also be declared with const since there is no need to re-assign that variable, and it doesn't need to be accessed in any context other than the function.

for (let e of questionBody.getElementsByTagName("*"))
    if (e.tagName !== "CODE" && e.tagName !== "PRE")
        e.innerHTML = replaceHTMLEntitiesWithRealCharacters(e.innerHTML);

Selecting elements

I typically see code that uses document.querySelectorAll() just to select items by tag or class name, but here I would recommend using querySelectorAll() with the CSS :not() selector because it can eliminate the need to have the if inside the loop and reduce the number of elements looped over.

For example in convertQuestionBodyToHTML()

for (let e of questionBody.querySelectorAll('*:not(code):not(pre)'))
    e.innerHTML = replaceHTMLEntitiesWithRealCharacters(e.innerHTML);

And the same applies to convertAnswersBodiesToHTML()

The function highlightCodeInsideElement() could be simplified to only select elements that are <code> elements with a parent element that is a <pre> element using the CSS child combinator:

const codeElements = element.querySelectorAll("pre > code");

for (let c of codeElements)
    hljs.highlightBlock(c);

It may be appropriate to use codeElements.forEach(highlightBlock) unless the parameters don't align properly.

Replacing characters

The function replaceHTMLEntitiesWithRealCharacters() appears to decode HTML entities by replacing three characters. Each call to replaceAll splits the string with the search string and joins using the replace string as the glue. Did you consider using String.replace() with a regex? I my experiments it seems faster to use regular expressions to

function replaceHTMLEntitiesWithRealCharacters(string) {
    string = string.replace(/&lt;/g, "<");
    string = string.replace(/&gt;/g, ">");

    // This HTML entity should be the last since
    // it can affect on the other entities.
    string = string.replace(/&amp;/g, "&");
    return string;
}

As this SO answer suggests the DOMParser API could be used but it seems to be a bit slower, likely because it does more than just replacing those three characters.

Event handlers

The function createRendererTagElement() creates span tags with an svg element that has an onmouseup event handler. It is better to use Element.addEventListener() for multiple reasons:

  • separation of JS logic from HTML
  • allows multiple event handlers if necessary
\$\endgroup\$
3
  • \$\begingroup\$ Thanks you very much for the answer! I would like to give a bounty for your answer, too. But I do not see a «start bounty» button anymore. Is it possible to give two bounties under the single one question? \$\endgroup\$
    – eanmos
    Jun 3, 2020 at 10:07
  • \$\begingroup\$ I wasn't expecting to earn the bounty as I didn't really answer any of the main questions you had. Either it won't let you because it hasn't been long enough, or you don't have enough reputation, since "if you offer multiple bounties on the same question, the minimum spend doubles with each subsequent bounty" so you'd have to offer 600 reputation. Another user could be convinced to offer a bounty in your place. \$\endgroup\$ Jun 3, 2020 at 16:43
  • \$\begingroup\$ Anyway, you spent your time to answer my question and it helped me a lot. I really appreciate it. Thank you a lot, again! \$\endgroup\$
    – eanmos
    Jun 3, 2020 at 17:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.