2
\$\begingroup\$

I coded this function to check for a violation of a primary key in a DataTable. The main idea is to retrieve a list of every row that the same value of the column in input and set an error to it.

Private Sub PrimaryKeyCheck(Of ColumnType)(dt As DataTable, ColumnName As String)

    Dim violatingGroups =
        dt.AsEnumerable() _
        .Where(Function(i) i.RowState <> DataRowState.Deleted) _
        .GroupBy(Function(i) i.Field(Of ColumnType)(ColumnName)) _
        .Where(Function(g) g.Count() > 1).ToList()

    For Each violatingGroup In violatingGroups
        For Each violatingRow In violatingGroup
            violatingRow.SetColumnError(ColumnName, "Error, Violating PK") ' TODO Traduzione
        Next
    Next

End Sub

I'm a newbie of Linq, so I'm asking. Is it possible to improve this function in terms of performance and correctness? Is it possible to remove the double for each?

\$\endgroup\$
  • \$\begingroup\$ Is there a reason you don't enforce unique constraints in the DataTable itself? \$\endgroup\$ – Comintern Nov 14 '15 at 15:20
  • \$\begingroup\$ Welcome to Code Review! To get the best out of your experience you should update the title of your question to describe what the code does, as code improvement is implied by all questions on this site. I hope you get good answers! \$\endgroup\$ – Der Kommissar Nov 14 '15 at 15:25
  • \$\begingroup\$ @Comintern There isn't a particular reason, I was "studying" how to implement a check by myself \$\endgroup\$ – Francesco Bonizzi Nov 14 '15 at 16:32
3
+50
\$\begingroup\$

According to the .NET Naming Guidelines, parameter names should be written in lower Camel case and be descriptive.

Private Sub PrimaryKeyCheck(Of ColumnType)(table As DataTable, columnName As String)

"Is it possible to remove the double for each?"

Yes! There's a handy method in the List<T> class named ForEach. The provided action will be invoked for each element in the list.

I advice you to not use the name i in this context as this name is widely used as an abbreviation for index (usually a number). A better name is r or row. And g could be renamed to group.

You should also move the magic string "Error, Violating PK" to a constant.

Const errorMessage As String = "Error, Violating PK"

table _
    .AsEnumerable() _
    .Where(Function(r) r.RowState <> DataRowState.Deleted) _
    .GroupBy(Function(r) r.Field(Of ColumnType)(columnName)) _
    .Where(Function(g) g.Count() > 1) _
    .ToList() _
    .ForEach(Sub(g) g.ToList().ForEach(Sub(r) r.SetColumnError(columnName, errorMessage)))
\$\endgroup\$
1
\$\begingroup\$

While Bjørn-Roger Kringsjå certainly gives you an option, I believe there is a better way for your for each issue.

Since you are doing the same thing to every item that matches your criteria, you can join the groups into the same collection. Use SelectMany to get each row directly and use a single for each:

Dim violatingRows =
    dt.AsEnumerable() _
    .Where(Function(i) i.RowState <> DataRowState.Deleted) _
    .GroupBy(Function(i) i.Field(Of ColumnType)(ColumnName)) _
    .Where(Function(g) g.Count() > 1) _
    .SelectMany(Function(g) g) 'No need to call ToList, but you can if you wish.

For Each violatingRow In violatingRows
    violatingRow.SetColumnError(ColumnName, "Error, Violating PK") ' TODO Traduzione
Next

Here's a good example of the difference between select and selectmany

\$\endgroup\$
  • 1
    \$\begingroup\$ @Bjørn-RogerKringsjå Fair enough, I guess he did say performance and correctness. I was focusing on the code that is written, not the code that is ran. I really have no idea which would be faster without timing it. \$\endgroup\$ – Daniel Dec 4 '15 at 17:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.