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

Question

お世話になります。

拡張性の高いクラスというものを作りたく
ジェネリックを使用しクラスを作成してみました。

このような作り方じゃだめ
こうするべき
これを参考にしたほうがいい
等ありましたらご教授ください

どうかよろしくお願いいたします。

追記１
１、２言のコメントでも頂けましたら幸いです。
今後のプログラミングでいかしていけるよう心に留めます。

概要
    リスト内要素 2回以上連続する場合1回とする
クラス   
    Class ConvertList(Of T)
コンストラクタ
    Sub New(ByVal list As List(Of T), ByVal convertType As Type, Optional ByVal itemName As String = Nothing)
メソッド 
    Function GetList() As List(Of T)

[使用例]
1.
    Dim list as new List(of DataRow)
       Dim row as DataRow = nothing
       Dim column as new DataColumn("F_Tmp",Type.GetType("System.String"))
       Dim table as DataTable("table1")

       table.Columns.Add(column)

       For i = 0 to 5
           row = table.NewRow
           row.Item("F_Tmp") = "aaa"
           list.Add(row)
       next

       For i = 0 to 10
           row = table.NewRow
           row.Item("F_Tmp") = "bbb"
           list.Add(row)
       next

       For i = 0 to 3
           row = table.NewRow
           row.Item("F_Tmp") = "ccc"
           list.Add(row)
       next

       Dim cls As New ConvertList(Of DataRow)(list, ConvertList(Of DataRow).Type.NotConsecutive, "F_Tmp")
       
       '列名 F_Tmp を対象とし、連続しているDataRowを1つにする
    list = cls.GetList()

       'リストの中身はaaa,bbb,ccc１つずつになる
    
2.
    Dim list As New List(Of String) From {"one", "one", "two", "three", "three", "three"}
       
       Dim cls As New ConvertList(Of DataRow)(list, ConvertList(Of DataRow).Type.NotConsecutive)

      'ToStringを対象とし、連続しているStringを１つにする
       list = cls.GetList()

    'リストの中身はone,two,three１つずつになる

以下本体

''' <summary>
''' リストを変換する
''' </summary>
''' <remarks></remarks>
Public Class ConvertList(Of T)

#Region "定義"

    ''' <summary>
    ''' 変換タイプ
    ''' </summary>
    ''' <remarks></remarks>
    Enum Type
        '連続しない
        NotConsecutive
    End Enum

#End Region

#Region "変数"

'リスト
Private Property list As List(Of T)

'対象アイテム
Private Property itemName As String

'変換タイプ
Private Property convertType As Type

'変換クラス
Private Property cls As IConvertEnumrable(Of T)

#End Region

#Region "コンストラクタ"

''' <summary>
''' コンストラクタ
''' </summary>
''' <param name="list">変換対象のリスト</param>
''' <param name="convertType">変換タイプ</param>
''' <param name="itemName">リスト内各要素 比較対象</param>
''' <remarks></remarks>
Public Sub New(ByVal list As List(Of T), ByVal convertType As Type, Optional ByVal itemName As String = Nothing)
    Me.list = list
    Me.itemName = itemName
    Me.convertType = convertType

    '変換タイプの変換クラスをインスタンス化
    Call SetConvertType()
End Sub

#End Region

#Region "公開メソッド"

 ''' <summary>
 ''' 行返す
 ''' </summary>
 ''' <returns></returns>
 ''' <remarks></remarks>
Public Function GetList() As List(Of T)

    '行取得
    Return Me.cls.GetEnumerable().ToList()

End Function

#End Region

#Region "内部メソッド"

    ''' <summary>
    ''' 変換タイプに一致するクラスをインスタンス化
    ''' </summary>
    ''' <remarks></remarks>
    Private Sub SetConvertType()

        Select Case Me.convertType
            Case Type.NotConsecutive
                    Me.cls = New NotConsecutive(Of T)()
                    Me.cls.SetEnumerable(Me.list, Me.itemName)
        End Select

    End Sub

#End Region

End Class

''' <summary>
''' 変換インターフェイス
''' </summary>
''' <remarks></remarks>
Public Interface IConvertEnumrable(Of T)

    'リストを受け取る
    Sub SetEnumerable(ByVal list As IEnumerable(Of T), ByVal itemName As String)

    'リストを返す
    Function GetEnumerable() As IEnumerable(Of T)

    '比較対象となるアイテム取得
    Function GetItem(ByVal i As Integer) As String

End Interface



''' <summary>
''' 変換クラス
''' </summary>
''' <remarks></remarks>
Class NotConsecutive(Of T)
    Implements IConvertEnumrable(Of T)

#Region "変数"

    'リスト
    Public Property list As List(Of T)
    '対象アイテム
    Public Property itemName As String

#End Region
  
#Region "公開メソッド"

    ''' <summary>
    ''' リストを受け取る
    ''' </summary>
    ''' <param name="list"></param>
    ''' <param name="itemName"></param>
    ''' <remarks></remarks>
    Public Sub SetEnumerable(list As IEnumerable(Of T), ByVal itemName As String) Implements IConvertEnumrable(Of T).SetEnumerable

        Me.list = list
        Me.itemName = itemName

    End Sub

    ''' <summary>
    ''' リストを返す
    ''' </summary>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Iterator Function GetEnumerable() As IEnumerable(Of T) Implements IConvertEnumrable(Of T).GetEnumerable

        For i = 0 To Me.list.Count - 1

            If i <> Me.list.Count - 1 Then
                '最後のインデックス以外
                While GetItem(i) = GetItem(i + 1)
                    '連続する値は返さない
                    Continue For
                End While
            End If

            '連続時は最後の値を返す
            Yield Me.list(i)
        Next

     End Function

     ''' <summary>
     ''' アイテム取得
     ''' </summary>
     ''' <param name="i"></param>
     ''' <returns></returns>
     ''' <remarks></remarks>
     Public Function GetItem(ByVal i As Integer) As String Implements IConvertEnumrable(Of T).GetItem
        Dim item As Object = Me.list(i)     'インデックスの位置のアイテム
        Dim ret As String = Nothing         '戻り値

        If Me.itemName Is Nothing Then
            'アイテムの指定がなかった場合
            ret = item.ToString()
        Else
            'アイテムの指定があった場合
            Select Case GetType(T)
                '- DataRowの場合
                Case GetType(DataRow)
                    ret = item.item(itemName)

            End Select
        End If

        '比較対象を返す
        Return ret
     End Function

#End Region

End Class

追記２
変換クラスである「NotConsecutive」において
連続を省くという処理が分離されておらず
良くないコードと感じましたので修正いたします。

''' <summary>
''' 変換クラス
''' </summary>
''' <remarks></remarks>
Class NotConsecutive(Of T)
    Inherits ConvertEnumrable(Of T)

#Region "公開メソッド"

    ''' <summary>
    ''' 連続する値は返さない
    ''' </summary>
    ''' <param name="i"></param>
    ''' <returns>T 返却対象 F 返却対象でない</returns>
    ''' <remarks>引数はリストの各インデックスである</remarks>
    Protected Overrides Function IsGetEnumrable(ByVal i As Integer) As Boolean

            If i <> Me.list.Count - 1 Then
                '最後のインデックス以外
                While GetItem(i) = GetItem(i + 1)
                    '連続する値は返さない
                    Return False
                End While
            End If

            Return True

    End Function

#End Region

End Class

あわせてインターフェース、変換クラスのベースを作成します。
(インターフェースにふさわしくないメソッドがあり修正いたしました。）

''' <summary>
''' 変換インターフェイス
''' </summary>
''' <remarks></remarks>
Public Interface IConvertEnumrable(Of T)

    'リストを受け取る
    Sub SetEnumerable(ByVal list As IEnumerable(Of T), ByVal itemName As String)

    'リストを返す
    Function GetEnumerable() As IEnumerable(Of T)

End Interface


''' <summary>
''' 変換クラス
''' </summary>
''' <typeparam name="T"></typeparam>
''' <remarks></remarks>
Public MustInherit Class ConvertEnumrable(Of T)
    Implements IConvertEnumrable(Of T)

#Region "変数"

    'リスト
    Protected Property list As List(Of T)
    '対象アイテム
    Protected Property itemName As String

#End Region

#Region "公開メソッド"

 ''' <summary>
    ''' リストを受け取る
    ''' </summary>
    ''' <param name="list"></param>
    ''' <param name="itemName"></param>
    ''' <remarks></remarks>
    Public Sub SetEnumerable(list As IEnumerable(Of T), ByVal itemName As String) Implements IConvertEnumrable(Of T).SetEnumerable

        Me.list = list
        Me.itemName = itemName

    End Sub

    ''' <summary>
    ''' リストを返す
    ''' </summary>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Iterator Function GetEnumerable() As IEnumerable(Of T) Implements IConvertEnumrable(Of T).GetEnumerable

     For i = 0 To Me.list.Count - 1

            '返却対象でない場合スキップ
            If IsGetEnumrable(i) = False Then Continue For

            '値を返す
            Yield Me.list(i)
     Next

    End Function

#End Region

#Region "内部メソッド"

    ''' <summary>
    ''' 返却対象の値であるか確認
    ''' </summary>
    ''' <param name="i"></param>
    ''' <returns>T 返却対象 F 返却対象でない</returns>
    ''' <remarks>引数はリストの各インデックスである</</remarks>
    Protected MustOverride Function IsGetEnumrable(ByVal i As Integer) As Boolean


    ''' <summary>
     ''' アイテム取得
     ''' </summary>
     ''' <param name="i"></param>
     ''' <remarks></remarks>
     Protected Function GetItem(ByVal i As Integer) As String
        Dim item As Object = Me.list(i)     'インデックスの位置のアイテム
        Dim ret As String = Nothing         '戻り値

        If Me.itemName Is Nothing Then
            'アイテムの指定がなかった場合
            ret = item.ToString()
        Else
            'アイテムの指定があった場合
            Select Case GetType(T)
                '- DataRowの場合
                Case GetType(DataRow)
                    ret = item.item(itemName)

            End Select
        End If

        '比較対象を返す
        Return ret
     End Function

#End Region

End Class

Answer 1

あまり構造化が得意ではないC#PGですが何点か。

1. そもそも設計としておかしい。

おそらく、実現しようとしている機能自体は本題じゃないからなんだと思うのですが
むりやり面倒なことをしていなぁ・・・と感じてしまいます。
IsGetEnumrableメソッドをデリゲートで受け付けてリストに反映してくれる拡張メソッドのほうが汎用性が高いのではないでしょうか？(ジェネリック関係なくなってしまうのでとりあえずこれはそういう方法がある程度にどうぞ)

1. 内部で作成されるインスタンスについて

「汎用性」が何を指しているかわかりませんが、「NotConsecutive」クラスはConvertListの外で定義されているのにConvertListの内部でインスタンスを作成しているのが気になります。
やるならType列挙型で動作を制限するより、動作そのものを外部から受け取るのが良いと思います。

1. 型依存

ConvertEnumrable.GetItemメソッドの内部で型による分岐が発生している為、果たしてこれは汎用性があるのか疑問です。
型依存するならば依存することを利用者に明示できるように型パラメータの制約で表現したほうが良いと思います。コメントもないですし、あえて隠しているならばいじわるです。

あとがき：
正直何をどこまでしたいのかわかりませんでしたが、手段のために目的を設定しているなぁという印象です。作者さんが「こういうものです」といえば「あ、はい」としかいえませんが；
「汎用性」というものをどこまで求めていらっしゃるかによりますが、少なくとも上記２と３の理由から汎用的とは言えないと感じております。
汎化するのと汎用性を求めるのは違うという認識の元でですが、「汎用性」を確保する為にクラスを設計するという方はあまり居ないのではないでしょうか。
あまりお役に立てなくて申し訳ありません。

Comments

[Ryzna]

蛇足かもしれませんが、型パラメータと汎用性についての私見です。

型パラメータ：
型パラメータは制約の有無を含めて型を明確にするために用いるべきと考えます。
制約をしないのであれば一貫して制限を設けない。
特定の結果をメンバがほしいならばそれが実装されていることを保障する特定のクラスなりインターフェイスなりを継承していることを制約とするべきと考えます。(自作が必要ならばそれも含めて)

汎用性：
今回のパターンでは型毎に挙動が変わるならばそれは同一クラスにするべきではないと考えます。
「ConvertList クラス は AリストをBリストに変換(投影)する際の基本的な構造・フローを定義するだけにとどめ、そこから派生してリストAやBの具体的な仕様を盛り込んだり、AからBに変換する実装をする」ほうがConvertListクラスそのものについては「汎用性」が出てくるのではないでしょうか？
私はこれが「汎化と特化」の一部だと考えています。(間違ってたら指摘ください；；)

[Ryzna]

楽する為の設計するのが一番いい気がします。

[ranran]

回答いただけたかた全員に対してになりますが
面倒くさい質問に答えて頂けたこと、
心より感謝申し上げます。

ご指摘を真摯に受け止め
反省とともにより良いコードを記述できるよう学ばせて頂きます。

> 1. そもそも設計としておかしい。

ジェネリックを使用するため無理やり作成した面があります(^^;

確かに拡張メソッドを使ったほうがシンプルそうです
ジェネリックを意識しすぎたあまり、
拡張メソッドまで気が回りませんでした。

> 2. 内部で作成されるインスタンスについて

悩んだ個所であるのですが、
クラス内に列挙型で動作定義したほうが、
インテリセンスの補完により扱いやすいかなと思いました。

ですがよくよく考えてみますと、
その度にクラスを変更してビルドして,,,となると非効率ですね。

今後、動作が増えるような場合には
外部より貰うようにしようと思います。

> 3. 型依存

その通りです、
コーディングしながら汚い書き方と思いました。

ですがこれが自身の限界です。
これ以外に書き方が思いつきませんでした。

リスト内の型によって
datarowの場合、Item("")を対象に
Stringの場合、toString()を対象に
のようにしたい場合どのようにするべきでしょうか？

それとも前提がおかしいのでしょうか？

> あとがき

目的としましては、
自身の日ごろのコーディングはその場凌ぎのものが多いです。
そしてどんどん似たようなコードが増えていきます。
修正が大変になります。

できるだけ拡張性が高く、今後の修正にも対応できるような
コードを記述したく今回は「ジェネリック」にスポットを当てました。

> あまりお役に立てなくて申し訳ありません。

大変勉強になります。
私自身、痛感しておりますが頭が固く
日頃より無駄の多いコードになっていると感じております。

よりよい技術者になれるよう頑張りたいと思います。

追記のコメントありがとうございます。
私用によりパソコンの前より離れますので、
今晩改めて追記に対するコメント致します。

今後ともよろしくお願いいたします。

[ranran]

> 型パラメータは制約の有無を含めて型を明確にするために用いるべき
> クラスなりインターフェイスなりを継承していることを制約とするべき

なるほどと思わされました！
根本的にジェネリックの認識が間違っていたようです。

> 「汎化と特化」

普段意識しておりませんでした(^^;
ジェネリックについてもそうですが、
使い方、定義を自分の中でしっかりまとめようと思います。

本当にありがとうございました！

Answer 2

拡張性というか汎用性を持たせたいのかなと思ったので、
私なりに汎用性を得ていく過程をなんとなくで書いてみました。
C#で書きます。すみません。

あなたはList<int>に対して連続ダブりを除去したいなと思いました。
そこで例えばこういうコードを書きます。

var source = new List<int>(){1,2,2,3,3,3,1};
var result = new List<int>();
result.Add(source[0]);
for (int i = 1; i < source.Count; i++)
{
    if (source[i] != source[i - 1]))
        result.Add(source[i]);
}
 //expected:1,2,3,1

その後あなたは別なところでもこの処理をしたいなと思いました。
そこで例えば拡張メソッドにします。

var source = new List<int>(){1,2,2,3,3,3,1};
var result = source.DistinctUntilChanged();

public static List<int> DistinctUntilChanged<int>(this List<int> source)
{
    var result = new List<int>();
    result.Add(source[0]);
    for (int i = 1; i < source.Count; i++)
    {
        if (source[i] != source[i - 1]))
            result.Add(source[i]);
    }
    return result;
}

（まあこれだと要素数が0,1の時例外投げますが。）
更にその後あなたはint以外でも同じことをやりたくなりました。
そこでジェネリックを使うことにしました。

public static List<T> DistinctUntilChanged<T>(this List<T> source) where T : IEquatable<T>
{
    var result = new List<int>();
    result.Add(source[0]);
    for (int i = 1; i < source.Count; i++)
    {
        if (!source[i].Equals(source[i - 1]))
            result.Add(source[i]);
    }
    return result;
}

更にさらに
...
...

長い上に質問の答えになっていませんが、
汎用性は必要になった時に必要なだけ拡張すればいいと思います。

Comments

[ozwk]

あと細かいことですが
・Convert～クラス、インターフェース
一瞬メソッド名かと思いました。
クラス名なら
EnumerableConverter
などと名詞形にするといいと思います。

・IsGetEnumrable
Isの後にGetが来るのもどうなんでしょうね。
CanGet～とかいいんじゃないでしょうか。

まあ、「日本人しか読まないからいいんだよ！」と思うかもしれませんが。

[ranran]

回答いただけたかた全員に対してになりますが
面倒くさい質問に答えて頂けたこと、
心より感謝申し上げます。

ご指摘を真摯に受け止め
反省とともにより良いコードを記述できるよう学ばせて頂きます。

> 汎用性は必要になった時に必要なだけ拡張すればいいと思います。

拡張の過程をみさせていただきまさにその通りだと思いました！
すごいです、綺麗です！
このようにコードを考えていけばよいのですね

とても参考になりました！
自身でもすらすら記述できるよう
頭に刻み付けます。

> あと細かいことですが
> まあ、「日本人しか読まないからいいんだよ！」と思うかもしれませんが。

その中でもコメントいただけた、
ozwkさんに感謝を。

「名詞形」をまったく意識しておりませんでした、
今後、クラス名のつけ方、動詞、助動詞
意識していきます！

今後とも気づかれた点ありましたら是非
ご指摘ください♪

本当にありがとうございました！

連続除去メソッド、拡張メソッド、ジェネリックの
遷移がとても分かりやすかったです。
ベストアンサーと致します。