質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

新規登録して質問してみよう
ただいま回答率
85.50%
VB

VB(ビジュアルベーシック)はマイクロソフトによってつくられたオブジェクト指向プログラミング言語のひとつで、同社のQuickBASICが拡張されたものです。VB6の進化版といわれています。

C#

C#はマルチパラダイムプログラミング言語の1つで、命令形・宣言型・関数型・ジェネリック型・コンポーネント指向・オブジェクティブ指向のプログラミング開発すべてに対応しています。

.NET Framework

.NET Framework は、Microsoft Windowsのオペレーティングシステムのために開発されたソフトウェア開発環境/実行環境です。多くのプログラミング言語をサポートしています。

Q&A

解決済

2回答

2868閲覧

コードの評価をしてください

ranran

総合スコア85

VB

VB(ビジュアルベーシック)はマイクロソフトによってつくられたオブジェクト指向プログラミング言語のひとつで、同社のQuickBASICが拡張されたものです。VB6の進化版といわれています。

C#

C#はマルチパラダイムプログラミング言語の1つで、命令形・宣言型・関数型・ジェネリック型・コンポーネント指向・オブジェクティブ指向のプログラミング開発すべてに対応しています。

.NET Framework

.NET Framework は、Microsoft Windowsのオペレーティングシステムのために開発されたソフトウェア開発環境/実行環境です。多くのプログラミング言語をサポートしています。

0グッド

3クリップ

投稿2015/03/07 13:38

編集2015/03/08 13:05

お世話になります。

まだ経験も浅く若輩者ですが、精一杯知恵を絞り綺麗にコードをまとめました
これはだめだ、この書き方は見にくい、こうしたほうがもっといい等ありましたらご意見ください
コメント、変数名のつけ方、拡張性等も指摘いただければと思います。
(なおxmlコメント、エラーメッセージ、ログ等は省きました)
また私ならこう記述する等ありましたら合わせて教えてください。

補足 start
Linqを使えばらくだよ! との指摘がありました。 まったく持ってその通りです(^^;
記述したコードが悪かったのですが、データテーブルから取り出すうんぬんはこの際かまわないのです。
そちらよりも私のコーディングの構築の仕方のだめだしをしてください。
私が一から構築し考えたコードです、悪い癖、直したほうがいい書き方等あるかと存じます。
Linq等がなく、レコード単位でforでまわさないといけないと想定していただければと思います。
図々しく、分をわきまえぬ質問だと重々承知しておりますが、ご指南いただければ幸いです。
補足 end

作成物はデータテーブルより条件の各行を取得するクラスです
テーブル内容は以下のような状態とします

id name age place work group
1 tsuchiya 23 北海道 IT 2
2 tanaka 30 沖縄 美容師 2
3 satou 50 宮城 飲食店 300
4 simamura 32 愛知 訪問販売 300
5 matuda 34 青森 新聞配達 10
6 kimura 43 岡山 広告 10

取り出し条件は3種類
・全ての行
・年齢が指定数未満
・グループが指定桁未満

またチェック項目の情報は定数としてクラス内に持っているものとします
・年齢のカラム名
・年齢の上限
・グループのカラム名
・グループの桁数上限

''''''''' 以下コード ''''''''''

lang

1''''''''''''''''''''''''''''interface 開始'''''''''''''''''''''''''''''''''''''' 2 3Public Interface IGetDataBase 4 5 'フィルターを通したテーブル取得 6 Function GetDataTable() As DataTable 7 8 '行単位の処理 9 Function GetRows() As IEnumerable(Of DataRow) 10 11 '条件の確認 12 Function NoProblemRow(ByVal row As DataRow) As Boolean 13 14End Interface 15 16''''''''''''''''''''''''' Base クラス開始''''''''''''''''''''''''''''''''''''''''' 17 18Public Class GetDataBase : Implements IGetDataBase 19 20#Region "変数" 21 22 'テーブル 23 Public ReadOnly Property Table() As DataTable 24 Get 25 Return _Table 26 End Get 27 End Property 28 Private _Table As DataTable 29 30#End Region 31 32#Region "コンストラクタ" 33 34Sub New(ByVal table As DataTable) 35 Me._Table = table 36End Sub 37 38#End Region 39 40#Region "公開メソッド" 41 42#Region "フィルター通したテーブル取得" 43 44Public Function GetDataTable() As DataTable Implements IGetDataBase.GetDataTable 45 Dim ret As DataTable = Me.Table.Clone '戻り値 46 47 Try 48 49 '行の取得 50 For Each row As DataRow In GetRows() 51 ret.ImportRow(row) 52 Next 53 54 Return ret 55 Catch 56 Throw 57 End Try 58 59End Function 60 61#End Region 62 63#Region "行単位の処理" 64 65 Protected Iterator Function GetRows() As IEnumerable(Of DataRow) Implements IGetDataBase.GetRows 66 67 For Each row As DataRow In Me.Table.Rows 68 '各行に対しフィルターを実行 69 70 If NoProblemRow(row) = True Then 71 'フィルターを通った列を戻り値にセット 72 Yield row 73 End If 74 Next 75 End Function 76 77#End Region 78 79#Region "条件を満たしている行か判定" 80 81 Protected Overridable Function NoProblemRow(ByVal row As DataRow) As Boolean Implements IGetDataBase.NoProblemRow 82 Return False 83 End Function 84 85#End Region 86 87#End Region 88 89End Class 90 91 92'''''''''''''''''''''''''''''本体 クラス開始''''''''''''''''''''''''''''''''''''' 93 94 95Public Class GetData 96 Inherits GetDataBase 97 98#Region "定数" 99 100 'フィルター種類 101 Enum Filter 102 ALL = 0 '全データ対象 103 AGE_OVER_CHEAK '年齢未満がデータ対象 104 GROUP_LNG_CHEAK 'グループ桁数未満がデータ対象 105 End Enum 106 107 'チェック項目 108 Structure Cheak 109 110 '年齢 111 Const AGE As Integer = 30 112 Const AGE_COL_NAME As String = "age" 113 114 'グループ 115 Const GROUP_LNG As Integer = 2 116 Const GROUP_COL_NAME As String = "group" 117 118 End Structure 119 120#End Region 121 122#Region "変数" 123 124 '取得テーブル種類 125 Public ReadOnly Property TableFilter As Filter 126 Get 127 Return _TableFilter 128 End Get 129 End Property 130 Private _TableFilter As Filter 131 132#End Region 133 134#Region "コンストラクタ" 135 136 Sub New(ByVal Table As DataTable, Optional ByVal filter As Filter = GetData.Filter.ALL) 137 'テーブル格納 138 MyBase.New(Table) 139 'フィルターを格納 140 Me._TableFilter = filter 141 End Sub 142 143#End Region 144 145#Region "公開メソッド" 146 147 '行のチェック 148 Protected Overrides Function NoProblemRow(ByVal row As DataRow) As Boolean 149 Dim ret As Boolean = False '戻り値 150 151 Select Case Me.TableFilter 152 153 Case Filter.ALL 154 '全ての行が問題ない 155 ret = True 156 Case Filter.AGE_OVER_CHEAK 157 '条件を満たした年齢は問題ない 158 Dim tmpAge As String = row(Cheak.AGE_COL_NAME) 159 ret = IIf(IsAgeOver(tmpAge), False, True) 160 Case Filter.GROUP_LNG_CHEAK 161 '条件を満たしたグループ桁数の行は問題ない 162 Dim tmpGroup As String = row(Cheak.GROUP_COL_NAME) 163 ret = IIf(IsGroupOverLng(tmpGroup), False, True) 164 End Select 165 166 Return ret 167 End Function 168 169#End Region 170 171#Region "内部メソッド" 172 173 '年齢チェック 174 Private Function IsAgeOver(ByVal mAge As String) As Boolean 175 Dim ret As Boolean = False 176 177 '以上の場合T 178 If Convert.ToInt32(mAge) >= Cheak.AGE Then ret = True 179 180 Return ret 181 End Function 182 183 'グループの桁チェック 184 Private Function IsGroupOverLng(ByVal mGroupNum As String) 185 Dim ret As Boolean = False 186 187 '以上の場合T 188 If Convert.ToInt32(mGroupNum).ToString.Length >= Cheak.GROUP_LNG Then ret = True 189 190 Return ret 191 End Function 192 193#End Region 194 195 196 197End Class 198 199 200

気になる質問をクリップする

クリップした質問は、後からいつでもMYページで確認できます。

またクリップした質問に回答があった際、通知やメールを受け取ることができます。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

yohhoy

2015/03/08 12:38

コードレビューを依頼するなら、最低限、teratailのmarkdownによるフォーマッティングをした方がよいですね。現状だとそもそも(形式が崩れて)読みにくいです。
ranran

2015/03/08 13:10

まずはじめに最大限の感謝を。 大多数の方はこんなくだらないコードを、経験も浅い素人同然のプログラマが何ほざいてるんだと思われることでしょう。 そのなかで声をかけていただけたことうれしく思います。 コードとして囲ったのですが、このやり方でよろしいのでしょうか?
guest

回答2

0

ベストアンサー

VB.NETは使った事が無いので、モジュール/クラスの設計的な観点からのみコメントさせてもらいます。なお、絶対的な評価軸というものはありませんから、コメントは参考程度に留めてください。(納得できない/別の理由があるならそれでOK)

  • NoProblemRow()は名前に違和感があります。(少なくとも"NoProblem"はちょっとへん?)
  • GetData.GetDataTable()実装のTry構文は必要ですか?単に再Throwするだけなら冗長に思えました。
  • GetData.NoProblemRow()実装では条件分岐構造をとっていますが、せっかくクラス階層を分ける設計としたなら、各フィルタ別のサブクラスを継承/実装した方がよいのではないでしょうか?新しいフィルタの追加をしやすくなります。
  • 比較対象値を定数として内包していますが、クラス外部から与えられると汎用性が増します。(上記の専用フィルタクラスのコンストラクタ引数にするなど)
  • GetDataTable()はそもそも必要ですか?「全て通過するフィルタ」で取得できるデータと同じですよね?「特定データのみ取り出すフィルタをかけつつ、やっぱり全データも参照したいケースがある」というならそのままでも良いですが、処理意図と提供機能がすこし噛み合ない気もします。

命名規則やクラス設計については、まずはその言語の標準ライブラリを真似るのがベストです。また、基本的な部分は書けているように思えましたので、オープンソースプロジェクト等の他人が書いたソースコードを読む事をお勧めします。(といっても、VB.NETのOSSプロジェクトあるのかな…)

投稿2015/03/08 13:53

yohhoy

総合スコア6189

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

ranran

2015/03/08 14:49

回答ありがとうございます! 以下の二つには目からうろこが落ちました! ○Tryについて 今まで参加してきたプロジェクトでよく使われていたので、使ってしまいました よくよく考えればただThrowするだけなら必要ないものですよね! try文を使うとしたら、 ・finally処理が必要なとき ・ログ等エラー発生時になにかしらする必要があるとき といったところでしょうか? ○各フィルタ別のサブラスを継承/実装 なるほど! 断然そちらのほうが追加しやすいと思いました 普段インターフェース、継承、あまり使っていないので今後は意識してみます。 > 命名規則やクラス設計については、まずはその言語の標準ライブラリを真似るのがベストです。 標準ライブラリを真似るとはどうゆうことでしょう 内部コードを見ることができるということでしょうか? 今後とも先輩方のコードを拝見させていただき、質を上げていきたいと思います ありがとうございました!
yohhoy

2015/03/09 08:25

Try構文の利用シーンは仰るとおりですね。今回のコードでは冗長にみえたので指摘してみました。 「その言語の標準ライブラリを真似る」とは、クラス/インターフェイスやメソッドの設計や命名ルールを参考にするという意味でした。内部実装コードも参照できればベストですが、大抵はブラックボックス化されて提供されますから、実装の方法を参考にするのは難しいかもしれません。(あくまで学習用途としてなら、自分で再実装してみるというのもアリですね)
ranran

2015/03/09 09:58

自分だけでは気づけないことも多々ありますので、 指摘いただけたこと嬉しく思います。 さすがにブラックボックスですね(^^) ありがとうございました!
guest

0

絞込みするならLINQを使えばclassにしなくても良いかも。

VBわかんないのでC#です。
即興で作ったので細部にはご容赦を

lang

1 2DataTable dt = new DataTable(); 3 4#region データ格納 5dt.Columns.Add(new DataColumn("id", typeof(int))); 6dt.Columns.Add(new DataColumn("name", typeof(string))); 7dt.Columns.Add(new DataColumn("age", typeof(int))); 8dt.Columns.Add(new DataColumn("place", typeof(string))); 9dt.Columns.Add(new DataColumn("work", typeof(string))); 10dt.Columns.Add(new DataColumn("group", typeof(int))); 11 12Action<int, string, int, string, string, int> addRow = (id, name, age, place, work, group) => { 13 DataRow dr1 = dt.NewRow(); 14 dr1["id"] = id; 15 dr1["name"] = name; 16 dr1["age"] = age; 17 dr1["place"] = place; 18 dr1["work"] = work; 19 dr1["group"] = group; 20 dt.Rows.Add(dr1); 21}; 22#endregion 23// テストデータ追加 24addRow(1, "tsuchiya", 23, "北海道", "IT", 2); 25addRow(2, "tanaka", 30, "沖縄", "美容師", 2); 26addRow(3, "satou", 50, "宮城", "飲食店", 300); 27addRow(4, "simamura", 32, "愛知", "訪問販売", 300); 28addRow(5, "matuda", 34, "青森", "新聞配達", 10); 29addRow(6, "kimura", 43, "岡山", "広告", 10); 30 31// パラメータ 32FilterType ft = FilterType.Age; 33int paramAge = 10; 34int paramGroupLen = 2; 35 36// 絞込みLINQ 37var filtred = dt.Rows.OfType<DataRow>().Select(dr => { 38 return new { 39 id = (int)dr["id"], 40 name = (string)dr["name"], 41 age = (int)dr["age"], 42 place = (string)dr["place"], 43 work = (string)dr["work"], 44 group = (int)dr["group"], 45 }; 46}).Where(item => { 47 switch (ft) { 48 case FilterType.Age: 49 // 指定年齢未満 50 return paramAge > item.age; 51 case FilterType.Group: 52 // グループが指定桁数未満 53 return paramGroupLen > item.group.ToString().Length; 54 default: 55 // 全行返却 56 return true; 57 } 58}); 59foreach (var filterdRow in filtred) { 60 // ここに検索結果の行ごとの処理を書く 61}

投稿2015/03/07 14:19

退会済みユーザー

退会済みユーザー

総合スコア0

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

ranran

2015/03/07 15:18

回答ありがとうございます! 凄くきれいなコードです 自分が考え付きもしなかったコードの記述、関数の使い方 とてもいいものを見ることができました♪ 特に感動したのはwhereの中にswitchを書く書き方! その発想はなかったです(^^) さて本題です、 私の例に出したクラスもまずかったのですが、意図がお伝えできなかったようです Linqも存じていますが、今回は使用しませんでした。 そもそもの意図は綺麗なコード、保守性や拡張性をあげるためにどのようなコードを記述すればよいのかが知りたかったわけです。 次にステップアップするにはどうすればよいのかといいましょうか? もっといいコードを、価値のあるコードを記述したいと思っています そのように考え、プロの方にだめだししてほしく投稿しました linqを使ったほうがよいのはごもっともです、 ですがそちらは一時置いていただき、良くない癖、考え方、構築の仕方等指摘いただけないでしょうか? 若造ですが一プログラマとして誇りを責任を持ち、レベルの高いコーディングをしたいと思います。 せっかくお時間割いてコード書いていただいたにもかかわらず申し訳ありませんでした。
guest

あなたの回答

tips

太字

斜体

打ち消し線

見出し

引用テキストの挿入

コードの挿入

リンクの挿入

リストの挿入

番号リストの挿入

表の挿入

水平線の挿入

プレビュー

15分調べてもわからないことは
teratailで質問しよう!

ただいまの回答率
85.50%

質問をまとめることで
思考を整理して素早く解決

テンプレート機能で
簡単に質問をまとめる

質問する

関連した質問