バリデーション、DB検索などのエラーコードのまとめ方

実現したいこと

ログイン画面で、入力チェックをするバリデーションと入力したデータでDBを検索し、データを照合するチェックを行い、エラーがあればエラー画面へ、エラーがなければindex画面へ、と遷移させたいです。

質問詳細

現在の書き方だと、バリデーションとDB検索を一つのif文であらわしていますが、
エラー時、try-catch時と複数回エラー画面に遷移する書き方になるため
書き方が冗長気味なことが気になります。
もっとシンプルな書き方があれば、教えて頂けないでしょうか？

該当のソースコード

Java
1	 */
2	protected void doPost(HttpServletRequest request, HttpServletResponse response)
3			throws ServletException, IOException, NoResultException {
4
5		String code = request.getParameter("emp_number");
6		String password = request.getParameter("emp_pass");
7		String error = "";
8
9		EntityManager em = DBUtil.createEntityManager();
10
11		//入力エラーチェック
12		if (code == null || code.equals("")) {
13			//社員番号ブランクエラーによるエラー画面遷移
14			error = "社員番号がブランクです";
15			request.setAttribute("error", error);
16			RequestDispatcher rd = request.getRequestDispatcher("/WEB-INF/views/error/loginerrors.jsp");
17			rd.forward(request, response);
18
19		} else if (password == null || password.equals("")) {
20			//パスワードブランクエラーによるエラー画面遷移
21			error = "パスワードがブランクです";
22			request.setAttribute("error", error);
23			RequestDispatcher rd = request.getRequestDispatcher("/WEB-INF/views/error/loginerrors.jsp");
24			rd.forward(request, response);
25
26		} else {
27			//入力バリデーションクリア
28			try {
29				Employees employees = (Employees) em
30						.createNativeQuery("select * from employees where code =:code", Employees.class)
31						.setParameter("code", code)
32						.getSingleResult();
33
34				request.setAttribute("employees", employees);
35				RequestDispatcher rd = request.getRequestDispatcher("/WEB-INF/views/layout/index.jsp");
36				rd.forward(request, response);
37
38			} catch (NoResultException e) {
39				//検索データ0件
40				e.printStackTrace();
41				error = "社員番号未登録です";
42				request.setAttribute("error", error);
43				RequestDispatcher rd = request.getRequestDispatcher("/WEB-INF/views/error/loginerrors.jsp");
44				rd.forward(request, response);
45
46			} catch (NonUniqueResultException e) {
47				//検索データが複数件
48				e.printStackTrace();
49				error = "社員番号が複数あります";
50				request.setAttribute("error", error);
51				RequestDispatcher rd = request.getRequestDispatcher("/WEB-INF/views/error/loginerrors.jsp");
52				rd.forward(request, response);
53
54			}
55
56		}
57
58	}
59
60}

行動規範の内容に同意します

回答2件

ベストアンサー

もっとシンプルな書き方があれば

構造的な部分はともかくとして、シンプルな書き方のヒントはご自身が書いておられます。

「入力チェックをするバリデーションと入力したデータでDBを検索し、データを照合するチェックを行い、エラーがあればエラー画面へ、エラーがなければindex画面へ、と遷移」

です。
処理は大きく二つ、「入力チェックをするバリデーション」と「データを照合するチェックを行い、エラーがあればエラー画面へ、エラーがなければindex画面へ」。
前者には「エラーがあればエラー画面へ」を追加しても良いかもしれません。
これをそのままコード化し、各々の処理はメソッドにすればシンプル(な感じ)になるのでは無いでしょうか。

if(入力バリデーション()==エラー) {
エラー画面へ
}
if(データベース照合()==エラー) {
エラー画面へ
}
index画面へ

といった感じです。

投稿2023/02/09 15:19

jimbe

総合スコア13320

Pocchi

2023/02/10 07:55

前回に引き続き、今回も回答ありがとうございます。なるほど、ご指摘頂いた形で修正してみます。コメントして頂いて、大変助かります。

行動規範の内容に同意します

doPostメソッドにほぼ全て書いてしまっているのが原因に思います。
「単一責任の原則」というのがありますし、同時に「コントローラ」「モデル」なども学んでみましょう。

どのように組み立てるかを考えるのは、この手の概念を身に着けてからにした方が良いと思います（もしかしたら「オブジェクト指向」から）

今ある状態を「手法」から何とかしていこうとするとどこかでボロが出て修復している間にまた他で・・・となりがちなので、今のコードを一度捨てるくらいの気概を持ってもちょうどよいくらいと思います。
全体的にリファクタリングが必要に思いますし、やはり、機能を持たせすぎているのが最も気になります。

投稿2023/02/09 01:44

m.ts10806

総合スコア80888

Pocchi

2023/02/10 07:58

前回に引き続き、回答ありがとうございます。一応MVCモデルは考えて作成していましたが、理解が甘く、組み立ての仕方が分かっていないのが現状でした。全体的にリファクタリングしてみます。コメントをしていただいて、大変助かります。

m.ts10806

2023/02/10 08:21

バリデーションも外だし可能ですし、クエリ実行をコントローラ直に書いてるので、失礼ながら全くMVCの形を成してません。いわゆるファットコントローラになってます。如何に共通化できるか。コントローラをスッキリさせられるかが鍵かと思います。 APIをたくさん作って適宜使うようなイメージ。

Pocchi

2023/02/10 08:26

ありがとうございます。ご指摘の通りかと思います。 MVCモデルをざっとまねているだけで、深い理解は足りないのが現状です。共通化を外だしすることが苦手で、ファットコントローラーになってしまうのが悩みのたねです。 APIたくさん作る、ですね。部品化したら便利ですし、汎用性も高まります。汎用性を高められるよう、考えてコーディングしていきたいと思います。

m.ts10806

2023/02/10 08:55

まずは不変な情報と可変な情報を整理して引数でうまく対応するところから。「ルール、法則を作る」のも大事です。そうすると他のサービスがどのように実装されていそうかというのも想像できるようになってきます。

行動規範の内容に同意します

あなたの回答