5
\$\begingroup\$

The code I have wrote works fine, this inquiry being purely for educational purposes. I want to know how others would do this better and cleaner. I especially hate the way I add the list items to another list before they are joined. There has to be a more efficient way.

I realize an easy way to make this simple would be to store "OU=" and "DC=" in the database with their associated text, but that just feels unseemly to me.

I am building a string for the container argument of the PrincipalContext class for an LDAP call.

The "lst" List<string> contains DataRows of LDAP Organization Units like "Accounts", "Users", etc.

// Get ou list
List<string> lst = db.sda(sql).Rows.OfType<DataRow>().Select(dr => dr.Field<string>("txt")).ToList();

string OU = string.Empty;
List<string> lst = new List<string>();

foreach (string ou in Web.Info.Ldap.ouList)
{
    lst.Add(string.Format("OU={0}", ou));
}
OU = string.Join(",", lst); 

Result:

OU=Users,OU=Accounts,OU=Employees

I do the same thing to a list called dcList that produces the same kind of string:

DC = string.Join(",", lst); 

Result:

DC=severname,DC=another_value,DC=com

to which I join together with OU to get the complete string, like so:

string container = string.Join(",", OU, DC);

End result:

OU=Users,OU=Accounts,OU=Employees,DC=sever,DC=othervalue,DC=com

\$\endgroup\$
  • \$\begingroup\$ You're missing a semicolon at the end of the first line of code. Please copy/paste your code exactly as it is, thanks :) \$\endgroup\$ – mjolka Nov 28 '14 at 4:25
  • \$\begingroup\$ You should really fix typos in your code. If you want to show us multiple revisions of the same piece of code - you should split those into different code blocks, instead of putting them all together and creating a mess. Otherwise your question might get closed. Notice, that we deal with real code here, which works and compiles, not with pseudo-code. \$\endgroup\$ – Nikita B Nov 28 '14 at 7:07
5
\$\begingroup\$

String.Join can work with IEnumerable<T> so its not necessary to pass list to it. Same goes for foreach: you do not have to call ToList() at the end of your first query, you can loop through initial enumeration. With LINQ you can join strings like that:

//you can call `ToList()` at the end, if you need to cache query results, but you dont have to
var fields = db.sda(sql).Rows.OfType<DataRow>().Select(dr => dr.Field<string>("txt"));
//you can merge this Select with the previous one, if you are not going to re-use `fields` enumeration
var ou = fields.Select(f => "OU=" + f);
var dc = someOtherFields.Select(f => "DC=" + f);

var result = String.Join(",", ou.Concat(dc));
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

In addition to tinstaafl' answer you can also use the StringBuilder.Length property and turn this into a extension method like

public static String Join(this IEnumerable<String> list, String category, String delimiter)
{
    StringBuilder sb = new StringBuilder();
    foreach (String item in list)
    {
        sb.AppendFormat("{0}={1},", category, item);
    }
    if (list.Any()) { sb.Length -= 1; }
    return sb.ToString();
}  

This can be called like

String ou = Web.Info.Ldap.ouList.Join("OU", ",");
String dc = Web.Info.Ldap.dcList.Join("DC", ",");
String container = String.Concat(ou, ",", dc);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I find the meaning of this method unclear. IMO, readability and comprehension trump speed. \$\endgroup\$ – ANeves Nov 28 '14 at 15:11
1
\$\begingroup\$

One simpler way to get the formatted strings is to use a StringBuilder and build the string in the loop instead of creating a separate list and joining it:

string MakeFormattedString(List<string> input, string category)
{
    StringBuilder sb = new StringBuilder();
    int i = 0;
    for (; i < input.Count - 1; i++ )
    {
        sb.AppendFormat(category + "={0},", input[i]);
    }
    sb.AppendFormat(category + "={0}", input[i]);
    return sb.ToString();
}

Then something like this should work:

 string OU = MakeFormattedString(Web.Info.Ldap.ouList, "OU");
 string DC = MakeFormattedString(Web.Info.Ldap.dcList, "DC");
 string container = string.Join(",", OU, DC);
| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ Instead of sb.AppendFormat(category + "={0},", input[i]); you could simply do sb.AppendFormat("{0}={1},",category, input[i]); \$\endgroup\$ – Heslacher Nov 28 '14 at 7:21
  • \$\begingroup\$ -1: function explodes if called with 0 inputs. \$\endgroup\$ – ANeves Nov 28 '14 at 15:10

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.