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

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

ただいまの
回答率

88.04%

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

解決済

回答 2

投稿 編集

  • 評価
  • クリップ 3
  • VIEW 2,337

score 130

お世話になります。

まだ経験も浅く若輩者ですが、精一杯知恵を絞り綺麗にコードをまとめました
これはだめだ、この書き方は見にくい、こうしたほうがもっといい等ありましたらご意見ください
コメント、変数名のつけ方、拡張性等も指摘いただければと思います。
(なお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種類
・全ての行
・年齢が指定数未満
・グループが指定桁未満   

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


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

''''''''''''''''''''''''''''interface 開始''''''''''''''''''''''''''''''''''''''

Public Interface IGetDataBase

    'フィルターを通したテーブル取得
    Function GetDataTable() As DataTable

    '行単位の処理
    Function GetRows() As IEnumerable(Of DataRow)

    '条件の確認
    Function NoProblemRow(ByVal row As DataRow) As Boolean

End Interface

''''''''''''''''''''''''' Base クラス開始'''''''''''''''''''''''''''''''''''''''''

Public Class GetDataBase : Implements IGetDataBase

#Region "変数"

    'テーブル
    Public ReadOnly Property Table() As DataTable
        Get
            Return _Table
        End Get
    End Property
    Private _Table As DataTable

#End Region

#Region "コンストラクタ"

Sub New(ByVal table As DataTable)
    Me._Table = table
End Sub

#End Region

#Region "公開メソッド"

#Region "フィルター通したテーブル取得"

Public Function GetDataTable() As DataTable Implements IGetDataBase.GetDataTable
        Dim ret As DataTable = Me.Table.Clone    '戻り値

    Try

        '行の取得
        For Each row As DataRow In GetRows()
          ret.ImportRow(row)
        Next

        Return ret
    Catch
      Throw
    End Try

End Function

#End Region

#Region "行単位の処理"

 Protected Iterator Function GetRows() As IEnumerable(Of DataRow) Implements IGetDataBase.GetRows

    For Each row As DataRow In Me.Table.Rows
    '各行に対しフィルターを実行

         If NoProblemRow(row) = True Then
         'フィルターを通った列を戻り値にセット
            Yield row
         End If
    Next
 End Function

#End Region

#Region "条件を満たしている行か判定"

 Protected Overridable Function NoProblemRow(ByVal row As DataRow) As Boolean Implements IGetDataBase.NoProblemRow
    Return False
 End Function

#End Region

#End Region

End Class


'''''''''''''''''''''''''''''本体 クラス開始'''''''''''''''''''''''''''''''''''''


Public Class GetData
    Inherits GetDataBase

#Region "定数"

    'フィルター種類
    Enum Filter
        ALL = 0             '全データ対象
        AGE_OVER_CHEAK      '年齢未満がデータ対象
        GROUP_LNG_CHEAK     'グループ桁数未満がデータ対象
    End Enum

    'チェック項目
   Structure Cheak

        '年齢
        Const AGE As Integer = 30
        Const AGE_COL_NAME As String = "age"

        'グループ
        Const GROUP_LNG As Integer = 2
        Const GROUP_COL_NAME As String = "group"

    End Structure

#End Region

#Region "変数"

    '取得テーブル種類
    Public ReadOnly Property TableFilter As Filter
        Get
            Return _TableFilter
        End Get
    End Property
    Private _TableFilter As Filter

#End Region

#Region "コンストラクタ"

    Sub New(ByVal Table As DataTable, Optional ByVal filter As Filter = GetData.Filter.ALL)
        'テーブル格納
        MyBase.New(Table)
        'フィルターを格納
        Me._TableFilter = filter
    End Sub

#End Region

#Region "公開メソッド"

    '行のチェック
    Protected Overrides Function NoProblemRow(ByVal row As DataRow) As Boolean
    Dim ret As Boolean = False     '戻り値

    Select Case Me.TableFilter

    Case Filter.ALL
        '全ての行が問題ない
        ret = True
    Case Filter.AGE_OVER_CHEAK
        '条件を満たした年齢は問題ない
        Dim tmpAge As String = row(Cheak.AGE_COL_NAME)
        ret = IIf(IsAgeOver(tmpAge), False, True)
    Case Filter.GROUP_LNG_CHEAK
        '条件を満たしたグループ桁数の行は問題ない
        Dim tmpGroup As String = row(Cheak.GROUP_COL_NAME)
        ret = IIf(IsGroupOverLng(tmpGroup), False, True)
    End Select

    Return ret
    End Function

#End Region

#Region "内部メソッド"

    '年齢チェック
    Private Function IsAgeOver(ByVal mAge As String) As Boolean
        Dim ret As Boolean = False

        '以上の場合T
        If Convert.ToInt32(mAge) >= Cheak.AGE Then ret = True

        Return ret
    End Function

    'グループの桁チェック
    Private Function IsGroupOverLng(ByVal mGroupNum As String)
        Dim ret As Boolean = False

        '以上の場合T
        If Convert.ToInt32(mGroupNum).ToString.Length >= Cheak.GROUP_LNG Then ret = True

        Return ret
    End Function

#End Region



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

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

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

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 過去に投稿した質問と同じ内容の質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

質問への追記・修正、ベストアンサー選択の依頼

  • yohhoy

    2015/03/08 21:38

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

    キャンセル

  • ranran

    2015/03/08 22:10

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

    コードとして囲ったのですが、このやり方でよろしいのでしょうか?

    キャンセル

回答 2

checkベストアンサー

+1

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

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

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/03/08 23:49

    回答ありがとうございます!

    以下の二つには目からうろこが落ちました!
    ○Tryについて
    今まで参加してきたプロジェクトでよく使われていたので、使ってしまいました
    よくよく考えればただThrowするだけなら必要ないものですよね!
    try文を使うとしたら、
    ・finally処理が必要なとき
    ・ログ等エラー発生時になにかしらする必要があるとき
    といったところでしょうか?

    ○各フィルタ別のサブラスを継承/実装
    なるほど!
    断然そちらのほうが追加しやすいと思いました
    普段インターフェース、継承、あまり使っていないので今後は意識してみます。

    > 命名規則やクラス設計については、まずはその言語の標準ライブラリを真似るのがベストです。
    標準ライブラリを真似るとはどうゆうことでしょう
    内部コードを見ることができるということでしょうか?

    今後とも先輩方のコードを拝見させていただき、質を上げていきたいと思います

    ありがとうございました!

    キャンセル

  • 2015/03/09 17:25

    Try構文の利用シーンは仰るとおりですね。今回のコードでは冗長にみえたので指摘してみました。

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

    キャンセル

  • 2015/03/09 18:58

    自分だけでは気づけないことも多々ありますので、
    指摘いただけたこと嬉しく思います。

    さすがにブラックボックスですね(^^)
    ありがとうございました!

    キャンセル

0

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

VBわかんないのでC#です。
即興で作ったので細部にはご容赦を
DataTable dt = new DataTable();

#region データ格納
dt.Columns.Add(new DataColumn("id", typeof(int)));
dt.Columns.Add(new DataColumn("name", typeof(string)));
dt.Columns.Add(new DataColumn("age", typeof(int)));
dt.Columns.Add(new DataColumn("place", typeof(string)));
dt.Columns.Add(new DataColumn("work", typeof(string)));
dt.Columns.Add(new DataColumn("group", typeof(int)));

Action<int, string, int, string, string, int> addRow = (id, name, age, place, work, group) => {
    DataRow dr1 = dt.NewRow();
    dr1["id"] = id;
    dr1["name"] = name;
    dr1["age"] = age;
    dr1["place"] = place;
    dr1["work"] = work;
    dr1["group"] = group;
    dt.Rows.Add(dr1);
};
#endregion
// テストデータ追加
addRow(1, "tsuchiya", 23, "北海道", "IT", 2);
addRow(2, "tanaka", 30, "沖縄", "美容師", 2);
addRow(3, "satou", 50, "宮城", "飲食店", 300);
addRow(4, "simamura", 32, "愛知", "訪問販売", 300);
addRow(5, "matuda", 34, "青森", "新聞配達", 10);
addRow(6, "kimura", 43, "岡山", "広告", 10);

// パラメータ
FilterType ft = FilterType.Age;
int paramAge = 10;
int paramGroupLen = 2;

// 絞込みLINQ
var filtred = dt.Rows.OfType<DataRow>().Select(dr => {
    return new {
        id = (int)dr["id"],
        name = (string)dr["name"],
        age = (int)dr["age"],
        place = (string)dr["place"],
        work = (string)dr["work"],
        group = (int)dr["group"],
    };
}).Where(item => {
    switch (ft) {
        case FilterType.Age:
            // 指定年齢未満
            return paramAge > item.age;
        case FilterType.Group:
            // グループが指定桁数未満
            return paramGroupLen > item.group.ToString().Length;
        default:
            // 全行返却
            return true;
    }
});
foreach (var filterdRow in filtred) {
    // ここに検索結果の行ごとの処理を書く
}

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/03/08 00:18

    回答ありがとうございます!

    凄くきれいなコードです
    自分が考え付きもしなかったコードの記述、関数の使い方
    とてもいいものを見ることができました♪

    特に感動したのはwhereの中にswitchを書く書き方!
    その発想はなかったです(^^)

    さて本題です、
    私の例に出したクラスもまずかったのですが、意図がお伝えできなかったようです
    Linqも存じていますが、今回は使用しませんでした。

    そもそもの意図は綺麗なコード、保守性や拡張性をあげるためにどのようなコードを記述すればよいのかが知りたかったわけです。

    次にステップアップするにはどうすればよいのかといいましょうか?

    もっといいコードを、価値のあるコードを記述したいと思っています

    そのように考え、プロの方にだめだししてほしく投稿しました

    linqを使ったほうがよいのはごもっともです、
    ですがそちらは一時置いていただき、良くない癖、考え方、構築の仕方等指摘いただけないでしょうか?

    若造ですが一プログラマとして誇りを責任を持ち、レベルの高いコーディングをしたいと思います。

    せっかくお時間割いてコード書いていただいたにもかかわらず申し訳ありませんでした。

    キャンセル

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

  • ただいまの回答率 88.04%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

関連した質問

同じタグがついた質問を見る