Is this Function Perfect?

[prabha_friend]

Function DCon(FieldToConcatenate As String, TableName As String, Criteria As String, Optional ConChar As Variant, Optional SkipSameValues As Variant) As String
Dim RecordsetToConcatenate As Recordset
Dim ConString As String
Set RecordsetToConcatenate = CurrentDb.OpenRecordset("SELECT * FROM " & TableName & " WHERE " & Criteria & ";")
While Not RecordsetToConcatenate.EOF
    With RecordsetToConcatenate
        If .AbsolutePosition = 0 Then
            ConString = .Fields(FieldToConcatenate).Value
        Else
            If IsMissing(SkipSameValues) Then
                ConString = ConString & IIf(Not IsMissing(ConChar), ConChar, ",") & .Fields(FieldToConcatenate).Value
            Else
                'Do Skipping
                If .AbsolutePosition = .RecordCount - 1 Then    'No Next Record Available (Last Record)
                    If InStr(ConString, .Fields(FieldToConcatenate).Value) = 0 Then
                        ConString = ConString & IIf(Not IsMissing(ConChar), ConChar, ",") & .Fields(FieldToConcatenate).Value
                    End If
                Else
                    Do Until (.Fields(FieldToConcatenate).Value <> ValueonNextRecord(RecordsetToConcatenate, FieldToConcatenate))
                        If .Fields(FieldToConcatenate).Value <> ValueonNextRecord(RecordsetToConcatenate, FieldToConcatenate) Then
                            GoTo MoveNext   'No need to put .movenext on "else" criteria
                        End If
                        .MoveNext
                    Loop
                    If InStr(ConString, .Fields(FieldToConcatenate).Value) = 0 Then
                        ConString = ConString & IIf(Not IsMissing(ConChar), ConChar, ",") & .Fields(FieldToConcatenate).Value
                    End If
                End If
            End If
        End If
MoveNext:
        .MoveNext
    End With
Wend
DCon = ConString
End Function

Kindly pinpoint if there is an error or suggest if you find something to be improved. Waiting for your confirmations to move into the codeRep

 

[MarkK]

Your code calls a function you have not posted...

[COLOR="Blue"]ValueonNextRecord[/COLOR](RecordsetToConcatenate, FieldToConcatenate)

...so I can't test it.

Also, to me, and at first glance, I don't know what these parameters are for...

Optional ConChar As Variant, Optional SkipSameValues As Variant

What type are they really? Also, IMO, you should provide defaults, like...

Optional ConChar As string = " ", Optional SkipSameValues As Boolean = True

hth

 

[Guus2005]

I can only tell you what i would do differently. From a quick glance:
I would use hungarian variable notation, which means: give your variables a type prefix like strTablename.
If you need to use GOTO. I would have programmed it differently. GOTO's only use is for error handling.
Like MarkK pointed out, i would give the optional parameters a default. Also i wouldn't use the variant type that often. It is a slow type because it represents all other types.

As to the function of this piece of code i can't say much. There is usually a way to circumvent this many nested if's. What should it do?

HTH

 

[MarkK]

Here's code that I think is simpler, and does a little bit more, for your consideration...

Function Concat( _
    Field As String, Table As String, _
    Optional Where As String, Optional OrderBy As String, _
    Optional Distinct As Boolean = True, Optional Delimiter As String = ", ") As String

    Dim sql As String
    Dim tmp As String
    
    If Len(Where) Then Where = "WHERE " & Where & " "
    If Len(OrderBy) Then OrderBy = "ORDER BY " & OrderBy & " "
    
    sql = _
        "SELECT " & IIf(Distinct, "DISTINCT ", "") & Field & " " & _
        "FROM " & Table & " " & _
        Where & OrderBy
    
    With CurrentDb.OpenRecordset(sql)
        Do While Not .EOF
            tmp = tmp & Delimiter & .Fields(0)
            .MoveNext
        Loop
        .Close
    End With
    
    If Len(tmp) Then Concat = Mid(tmp, Len(Delimiter))
End Function

I think it is advantageous to take more time to construct exactly the SQL you need, and thereby keep your loop simpler and faster.

 

[Galaxiom]

The arguments should be declared as ByVal.

+1 to avoiding the use of Variants and GoTo

Skipping repeats could be done more efficiently in the query by using Distinct and returning only the field to be concatenated.

While/Wend is usually considered deprecated. Use a Do Loop.

The With Block for RecordsetToContatenate could be outside the Loop.

I wouldn't use such long names in the function.

 

[Minty]

Is this not another version of Allen Browne's Concatenate function?
http://allenbrowne.com/func-concat.html