文字列の解析処理について

こんにちは。

与えられた文字列を分解する処理を行う下記のソースについて、ご意見を頂ければと思います。

###前提・実現したいこと
使用言語：Java

与えられる文字列には、「＆」または「｜」が含まれている可能性があり、半角、全角を問わず「＆＆」「｜｜」であれば文字として扱い、「＆」であれば「AND」、「｜」であれば「OR」として扱います。

例）
「A&＆B」→「A＆B」
「A＆&B」→「A&B」
「A＆B」→「A AND B」
「AB｜」→「AB OR」

また、上記のように変換した後でクエリとして実行し、取得結果に対しての突き合わせに利用する想定です。

例）
「A&&B｜C｜｜D」→「'A&B' OR 'C｜D'」としてクエリを実行
→取得結果に対して、「A&B」または「C｜D」のどちらに一致したか判定したい。

###発生している問題
上記を実現するために、「A&&B」であれば、「A&B」として配列に格納、「A&B」であれば「A」「&」「B」として配列に格納するような処理を考えました。
処理の記載は行ったのですが、どうしてもわかりづらいソースになってしまい、他に書きようはないのかと模索しています。

###ソースコード

		for (String str : this.wordList) {
					String tempStr = str.replace("＆", "&").replace("｜, "|");
			String checkStr = "";
			for (int i = 0; i < str.length(); i++) {
				indexA = tempStr.indexOf("&", i);
				indexO = tempStr.indexOf("|", i);
				if (-1 == indexA || -1 == indexO) {
					index = indexA == -1 ? indexO : indexA;
				} else {
					index = indexA < indexO ? indexA : indexO;
				}

				if (-1 == index) {
					if (0 == i) {
						arrayList.add(str);
						break;
					} else {
						if (-1 != wordStart) {
							checkStr += str.substring(i);
							array.add(checkStr);
						}
						break;
					}
				} else {
					if (-1 == wordStart) {
						wordStart = i;
					}
					if (index + 1 == str.length()) {
						if (wordStart == index) {
							arrayLlist.add(tempStr.substring(index, index + 1));
						} else {
							checkStr += str.substring(wordStart, index);
							arrayList.add(checkStr);
							arrayList.add(tempStr.substring(index, index + 1));
						}
						break;
					} else {
						if (tempStr.substring(index, index + 1).equals(tempStr.substring(index + 1, index + 2))) {
							if (i != index) {
								checkStr += str.substring(i, index);
							}
							i = index + 1;
							checkStr += str.substring(index + 1, index + 2);
						} else {
							if (wordStart == index) {
								arrayList.add(tempStr.substring(index, index + 1));
								wordStart = index + 1;
							} else {
								i = index;
								checkStr += str.substring(wordStart, index);
								arrayList.add(checkStr);

								arrayList.add(tempStr.substring(index, index + 1));
								wordStart = -1;
							}
						}
						if (str.length() == i + 1 && (-1 != wordStart)) {
							arrayList.add(checkStr);
						}
					}
				}

			}
		}

以上、宜しくお願い致します。

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

回答5件

ベストアンサー

・連続しない&, |で文字列を分割
・分割された文字列の中に&&, ||があれば&, |に置き換える
と分けると見通しが良くなると思います。

Java
1ArrayList<String> parse(String str)
2{
3	ArrayList<String>	arr = new ArrayList<String>();
4	int		top = 0;
5
6	str = str.replace("＆", "&");
7	str = str.replace("｜", "|");
8	
9	for(int i=0; i<str.length(); i++){
10		char	ch = str.charAt(i);
11		
12		if(ch == '&' || ch == '|'){
13			if(i != str.length()-1 && str.charAt(i+1) == ch){
14				// && or ||
15				i++;
16			}else{
17				putWord(arr, str, top, i);
18				arr.add(""+ch);
19				top = i+1;
20			}
21		}
22	}
23	
24	putWord(arr, str, top, str.length());
25	
26	return arr;
27}
28
29void putWord(ArrayList<String> arr, String str, int start, int end)
30{
31	if(start < end){
32		String	word = str.substring(start, end);
33		word = word.replace("&&", "&");
34		word = word.replace("||", "|");
35		arr.add(word);
36	}
37}

投稿2015/11/29 05:07

toki_td

総合スコア2850

kurage

2015/12/01 14:35

toki_td様：ご回答いただき、ありがとうございます。ご指摘の通りに処理を分けることで、元のソースよりも断然見通しの良いソースにすることができました。分断した後で書き換えるという発想がなかったので長々と記載していたのですが、処理を分けることでこんなにも違うとは思いませんでした。本当にありがとうございました。

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

アイデアとしては…
1.全角を半角に直す(これまでの回答者同様)
2."&&"で文字列をsplitして配列にする
3.配列の各要素に対し"&"を"AND"にreplaceAll
4.配列の要素を"&"で連結
5."||"でsplit
6.各要素の"|"を"OR"にreplaceAll
7.要素を"|"で連結
こんなのはどうでしょう？

投稿2015/11/30 15:57

swordone

総合スコア20651

kurage

2015/12/01 15:18

swordone様：ご回答いただき、ありがとうございます。 splitを使用することを思いつきませんでした。文字列の位置も気にしなくていいので、何をしているのかがわかりやすいソースを書くことができました。分割した文字を配列で保持して持ちまわりたかったので今回は別の方法で実装しましたが、このような方法もあるのだという選択肢を増やすことができました。ご教授くださり、ありがとうございました。

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

正直いって、よくわからないコードですね（＾＾；；
私も、昔？は力ずくでコードを書いていため、書いた後すぐは良いのですが、何日か経った後、見返すと良くわからないということが、多々ありました。
それからは、なるべく小さな機能にわけ、考えるようにしています。
そのときは、性能を考えず、分かりやすさを優先します。

本題の場合、以下のように考えます。
・全角の"＆"を半角の"&"に置き替える
・全角の"｜"を半角の"|"に置き替える
・連続していない1個の"&"を" AND "に置き替える
・連続していない1個の"|"を" OR "に置き替える
・"&&"を"&"に置き替える
・"||"を"|"に置き替える

とりあえずこれをそのまま関数にして、以下の第1版を作成します。

java
1for (String words : wordsList) {
2	// 全角の"＆"を半角の"&"に置き替える
3	// 全角の"｜"を半角の"|"に置き替える
4	words = replaceZenAnd(words);
5	words = replaceZenOr(words);
6	// 連続していない1個の"&"を" AND "に置き替える
7	// 連続していない1個の"|"を" OR "に置き替える
8	words = replaceOneAnd(words);
9	words = replaceOneOr(word);
10	// "&&"を"&"に置き替える
11	// "||"を"|"に置き替える		
12	words = replaceDowbleAnd(words);
13	words = replaceDowbleOr(words);	
14}

これで各機能の詳細（コード）を考え、頭のなかがスッキリしてきたところで、関数にする必要のないもの、合成した方よいものなどを検討して次の版を作成します。
以上です。

投稿2015/11/30 14:48

akiruno-oneone

総合スコア815

kurage

2015/12/01 15:04

akiruno-oneone様：ご回答いただき、ありがとうございます。自分でも書いていて分からなくなってきてしまい、今後保守する上でバグの温床になるのではと不安になるソースを書いてしまいました。処理を最小単位に分割して実装、その後の見直しでまとめたりなどの検討をしてゆけばよいのですね。一気に完成形に持って行こうとするのが誤りでした。今後コーディングを行う際には小さい単位で処理を考えるところから行うようにしていきます。ご教授くださり、ありがとうございました。

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

他に書きようはないのか

に関しては私の手には負えないので(w)他の回答者様に譲るとして、
可能であれば設計から見直すことをお勧めします。

ご質問の設計には、以下のような問題が考えられるからです。
・文字列の先頭や末尾に"&"や"|"が現れた場合、どうするのか？
・例えばA&&&Bという文字列は、'A&' AND 'B'とすべきか'A' AND '&B'とすべきか判断できない
・例えばA&(B|C)という文字列は'A' AND '(B' OR 'C)'と解析してしまうが、それで良いのか?
（'A' AND ('B' OR 'C')にならないけど良いのか？という意味です）

あと、解析後のクエリにはプレースホルダを使用するようにしましょう。
さもないと、SQLインジェクションの危険性があるからです。
https://ja.wikipedia.org/wiki/SQL%E3%82%A4%E3%83%B3%E3%82%B8%E3%82%A7%E3%82%AF%E3%82%B7%E3%83%A7%E3%83%B3

投稿2015/11/29 06:39

KiyoshiMotoki

総合スコア4791

kurage

2015/12/01 14:58

KiyoshiMotoki様：ご回答いただき、ありがとうございます。ご指摘の内容は先輩にも同様の指摘を受けて確認した結果、処理は変わらず、ユーザーがそのルールを意識するということになりました。（A&&&B → A& AND B）ユーザーにクエリを意識させるという点がおかしいとは思うのですが、設計者にそれでいいと断言されてしまいました。 SQLインジェクションの危険性については報告し、これから対応する予定です。実装にばかり気を取られていてSQLインジェクションや仕様についての確認がおろそかになっていました。ご教授くださり、ありがとうございました。

KiyoshiMotoki

2015/12/02 11:03

kurage様、ご丁寧な報告ありがとうございます。 > ユーザーにクエリを意識させるという点がおかしいとは思うのですが、設計者にそれでいいと断言されてしまいました。それは残念ですね。。しかし、これからもご自分が「これはおかしい」「こっちの方が良い」と思う事は、遠慮せずどんどん提案する事をお勧めします。そうする事で、kurage様の手がけるシステム/アプリがより良いものになる機会を作る事ができますし、 kurage様の技術力の向上にもつながると思うからです。お役に立てたなら幸いです。

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

半角のみしか考慮していませんが、「A&&B|C||D&E」を「 'A&B' OR 'C|D' AND 'E' 」という文字列に書き換えるプログラムを書いてみました。後続処理がよくわからなかったので、文字列にしていますが、適当にオブジェクトに入れると扱いやすくなるかと思います。

public class Parser1
{
public static void main(String[] args)
{
Parser1 parser = new Parser1();
String result = parser.parse("A&&B|C||D&E");
System.out.println(result);
}
public String parse(String target)
{
Pattern pattern = Pattern.compile("[^&]&[^&]+?|[^\|]\|[^\|]+?");
Matcher matcher = pattern.matcher(target);
StringBuilder sb = new StringBuilder();
int index = 0;
while(matcher.find())
{
sb.append(" '");
sb.append(target.substring(index, matcher.start() + 1));
sb.append("' ");
char operator = target.charAt(matcher.start() + 1);
switch(operator)
{
case '&' :
sb.append(" AND ");
break;
case '|':
sb.append(" OR ");
break;
default :
break;
}
index = matcher.start() + 2;
}
sb.append(" '");
sb.append(target.substring(index));
sb.append("' ");
return sb.toString().replaceAll("&&", "&").replaceAll("\|\|", "\|");
}
}