1
\$\begingroup\$

I am working on some coursework for a simple Java EE web application that records blog comments. On the add comment page there are two fields: name and comments

I am using a RequestDispatcher to pass control to an acknowledgement.jsp if the fields are not empty, and an error.jsp if one (or both) are empty passing a relevant message to the acknowledgement page.

This is the logic I have:

  RequestDispatcher disp;
  if (!name.isEmpty() && !comment.isEmpty())
  {
      CommentBean commentBean = new CommentBean();
      commentBean.setComment(comment);
      commentBean.setName(name);
      req.setAttribute("comment", commentBean);

      disp = req.getRequestDispatcher("acknowledgement.jsp");
      disp.forward(req, resp);

      return;
  } 
  else if (!name.isEmpty() && comment.isEmpty())
  {
      disp = req.getRequestDispatcher("error.jsp");
      statusMessage = "Comment field empty";
      req.setAttribute("status", statusMessage);
      disp.forward(req, resp);
  }
  else if (name.isEmpty() && !comment.isEmpty())
  {
      disp = req.getRequestDispatcher("error.jsp");
      statusMessage = "Name field empty";
      req.setAttribute("status", statusMessage);
      disp.forward(req, resp);
  }
  else
  {
      disp = req.getRequestDispatcher("error.jsp");
      statusMessage = "Name and comment fields empty";
      req.setAttribute("status", statusMessage);
      disp.forward(req, resp);
  }

The code does what I want but I can't help thinking this could be written a lot better although I am not sure how to go about it.

\$\endgroup\$
1
  • \$\begingroup\$ It;s a small remark, so I won;t post it as an answer: Use proper indentation -> brackets should start on the same line \$\endgroup\$ Sep 27, 2016 at 18:02

1 Answer 1

3
\$\begingroup\$

That's easy: You're repeating quite a few things, so don't do it.

disp.forward(req, resp);

This is the easiest, copied in each of the 4 branches, but needn't be. Drop the needless return and move it after the branches.

But in general, deduplication is hard and it's much better to learn to avoid duplications. Just imagine Ctrl-C and Ctrl-V are broken and you must type using one finger only. :D You could write a method accepting statusMessage and handling the problem. But it's simple enough so you can do it inline.

The basic idea is "compute the statusMessage, leave it empty in case there's no problem and handle it":

if (comment.isEmpty()) {
    statusMessage = name.isEmpty()
        ? "Name and comment fields empty"
        : "Comment field empty";
} else {
    statusMessage = name.isEmpty()
        ? "Name field empty"
        : "";
}

if (statusMessage.isEmpty()) {
    CommentBean commentBean = new CommentBean();
    commentBean.setComment(comment);
    commentBean.setName(name);
    req.setAttribute("comment", commentBean);
    disp = req.getRequestDispatcher("acknowledgement.jsp");
} else {
    disp = req.getRequestDispatcher("error.jsp");
}

disp.forward(req, resp);
\$\endgroup\$

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.