著作一覧 |
目が覚めるような工夫を見た。
ソースを眺める。... List list = TableMaster.read(connection, "foo", form.getBar(), form.getBaz()); boolean boolFlag = false; Iterator ite = list.iterator(); while (ite.hasNext()) { Map map = (Map)ite.next(); if (map.get("KEY3")).equals(form.getBoz())) { boolFlag = true; break; } } if (boolFlag == false) { ... } else { ... }
まあ、for (Iterator i = list.iterator(); i.hasNext();) と書かずに、わざわざループのスコープ外にイテレータを宣言するようなコード書くのにろくなのがいないということはわかってしまったから、どうせろくでもないことをしてるんだろうな、と見ていると、っていうかboolFlagかよ、どっからflagなんて言葉が出てくんだろうね? 一体としは幾つなんだか、とかむかむかしてきながら我慢して読んでいた。っていうか、fooってテーブルは何十万件だかあるテーブルなんだが、2つしかキーを与えないとへたすると数万とか返ってくるんじゃないか? 違うのかな? とか、まだ多少は希望はあったりしながら見ているわけだが。
... List list = (ArrayList)TableMaster.read(connection, "bogus", form.getBar(), form.getBaz()); boolean flag = false; Iterator iterator = list.iterator(); while (iterator.hasNext()) { Map map = (HashMap)iterator.next(); if (map.get("KEY3").equals(form.getFunky()) && map.get("KEY4").equals(form.getKinky()) && .... map.get("KEY8").equals(form.getDonky())) { flag = true; break; } } ...
今度のはもっとすげーな。flagかよ、っていうか、キャストがひど過ぎるが一体なんなんだ? っていうか、条件演算子を右端に書くやつにろくなのはいないっていうことはすでにお見通しなんだが、っていうかbogusってのは数100万件とかいう大物じゃなかったか? っていうか、KEY8って、おい……
てな調子で数本見ればさすがに馬も土曜に蒼ざめる。このおそるべきマスターはどこだ?
/** * 部品共通化 */ public class TableMaster { /** * 共通テーブル処理 */ public static List read(Connection c, String name, String key1, String key2) throws SQLException { StringBuilder sb = new StringBuilder(); sb.append("SELECT * FROM " + name + " WHERE id='" + key1); sb.append("' and subid='" + key2 + "'"); List list = new ArrayList(); PreparedStatement ps = null; ... try { ... while (rs.next()) { Map map = new HashMap(); ResultSetMetaData rsm = rs.getMetaData(); for (int i = 0; i < rsm.getColumnCount(); i++) { map.put(rsm.getColumnName(i + 1), rs.getObject(i + 1)); } list.add(map); } return list; } finally { ... } } }
うが。想像どおりなんですが。StringBuilder#appendの中で文字列連結してどうするよ、ってなことはその後のに比べりゃどうでもいいが(追加:良くねぇよ。キーを文字列連結しちゃいかんだろう。安全性の面からも実行効率の面からもPreparedStatement使うのが筋だ……って言うか使ってるよorz。使い方がぱっぱぱらひゃら)、これ最大同時予測アクセス数とか何も考えてないんだろうな、っていうかループの外に出せというかそんなことより、なぜ、こんな妙なユーティリティを作るのか? というか。1人も動かせばメモリーいっぱいいっぱいだというか、ああ、それでテストはしましたとか胸を張ったりするのかなというか、なんていうか、なんにも言う気も起きないんだが、*を使ってカラム全部とってもさっきまで見てた連中は残りのキーを除けば1つか2つのカラムしか見てないぞっていうか、そんなことは全体から見れば小さいことだが、それにしても各テーブルでできるだけカラム名を同じにするっていうことにこういう罠が待ってたわけですな。っていうか、Javadocコメントがすごく虚しいのだが。
汚染された廃物は再利用不可能だから書き直し決定。
ジェズイットを見習え |
TableMaster.read の key1, key2 がサニタイジングされてないのも気になる...
確かに。それも入れときます。
throws SQLException と finally の関係がちょっと気になる。
私は、よく StringBuilder#append の中で文字列連結してます(汗)。
> throws SQLException と finally の関係が<br>そこは、ちゃんとcloseするものさえfinallyの中でcloseしてればOK。例外をスローしてもリソースを確保したままっていう実装もあり得るから。<br>というか、どこが気になります?<br>>appendの中の文字連結<br>長い直定数の文字列を+で連結して複数行に書くのはOKですよ。意味は変わらないし、むしろ直定数として記述した文字列をappendで分けるのは意味ないし。もちろん変数が出て来るなら考え直すべきですけど。
read メソッドの引数として Connection c が呼び出し側から与えられているのに read メソッドの内部で close するの?とか、finally があるのなら SQLException も catch すべきではないか?とか思うのですが、いろいろと憶測が入っています。
インターフェイスとしてリソースを外部から与えられた場合は外部が生成/廃棄の責任を持つべきと思います。したがってreadメソッドがクローズの責任を持つのは内部で生成するPreparedStatementやResultSetです(外部からは見えないですね。一般論としては)。<br>finallyは上記の責任を果たすために必須ですが、例外の後始末についての責任をどこが果たすかはインターフェイスによって決まります。readはthrows SQLExceptionと宣言していることから、責任を取りません。呼び出し側がどうにかすべきと宣言しているので、catchする必要はありません。<br>で、答えになってると思うけど。
すみません。Connection の close と勘違いしてました。finally は Statement や RsultSet の close 用なのですね。納得です。