patterncsharpMinor
LDAP search helper for System.DirectoryServices.Protocols
Viewed 0 times
searchhelpersystemldapdirectoryservicesforprotocols
Problem
Given all the times I need to perform LDAP searches in the work I do, I wrote a helper and I want to clean it up and publish it to Nuget. What are peoples opinions of the approaches used?
```
namespace MyCompany.Directory.LDAP
{
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
///
/// Represents an LDAP entry.
///
public class LdapEntry : IReadOnlyDictionary
{
///
/// Private backing field for Attributes.
///
private readonly ReadOnlyDictionary attributes;
///
/// Private backing field for DistinguishedName.
///
private readonly string distinguishedName;
///
/// Initializes a new instance of the structure.
///
/// The distinguished name for the entry.
/// The attributes for the entry.
public LdapEntry(string distinguishedName, IDictionary attributes)
{
this.distinguishedName = distinguishedName;
this.attributes = new ReadOnlyDictionary(attributes);
}
///
/// Gets the attributes for the entry.
///
/// The s for the .
public ReadOnlyDictionary Attributes => this.attributes;
///
/// Gets the number of elements in the collection.
///
public int Count => this.attributes.Count;
///
/// Gets the distinguished name for the entry.
///
/// The distinguished name for the entry.
public string DistinguishedName => this.distinguishedName;
///
/// Gets an enumerable collection that contains the attribute types in the .
///
public IEnumerable Keys => this.attributes.Keys;
///
/// Gets an enumerable collection that contains the s in the .
///
public IEnumerable Values => this.attributes.Values;
///
/// Gets the that has the specified a
```
namespace MyCompany.Directory.LDAP
{
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
///
/// Represents an LDAP entry.
///
public class LdapEntry : IReadOnlyDictionary
{
///
/// Private backing field for Attributes.
///
private readonly ReadOnlyDictionary attributes;
///
/// Private backing field for DistinguishedName.
///
private readonly string distinguishedName;
///
/// Initializes a new instance of the structure.
///
/// The distinguished name for the entry.
/// The attributes for the entry.
public LdapEntry(string distinguishedName, IDictionary attributes)
{
this.distinguishedName = distinguishedName;
this.attributes = new ReadOnlyDictionary(attributes);
}
///
/// Gets the attributes for the entry.
///
/// The s for the .
public ReadOnlyDictionary Attributes => this.attributes;
///
/// Gets the number of elements in the collection.
///
public int Count => this.attributes.Count;
///
/// Gets the distinguished name for the entry.
///
/// The distinguished name for the entry.
public string DistinguishedName => this.distinguishedName;
///
/// Gets an enumerable collection that contains the attribute types in the .
///
public IEnumerable Keys => this.attributes.Keys;
///
/// Gets an enumerable collection that contains the s in the .
///
public IEnumerable Values => this.attributes.Values;
///
/// Gets the that has the specified a
Solution
-
You have too much unnecessary comments such as - Private backing field for ..., you don't really care about the backing field of a property anyway.
-
Convert your properties into auto properties like this:
Can be shorten to:
You can do the same for all of your properties.
-
This looks pretty crazy
Of course you should come up with proper names for the properties.
You can adapt this to your other classes e.g:
You can make the constructor accept
-
Here is an example of how
As you can see I've flipped the
-
There are some places where you can use the ternary operator instead of
Can be written like this:
-
To me this isn't clear:
Also your second exception "Directory returned to many search results during ranged retrieval." seems kinda odd, are you excluding the possibility of having a item with value null? This is how I would write it:
```
var searchResu
You have too much unnecessary comments such as - Private backing field for ..., you don't really care about the backing field of a property anyway.
-
Convert your properties into auto properties like this:
private bool rangedRetrieval;
public bool RangedRetrieval
{
get => return rangedRetrieval;
set => rangedRetrieval = value;
}Can be shorten to:
public bool RangedRetrieval { get; set; }You can do the same for all of your properties.
-
This looks pretty crazy
Dictionary, List>> and you're using this in a few places so it might be a good idea to make Tuple, List> a class, the type name will be shorter and you will actually have normal names instead of Item1 and Item2. Say:public class RawAttribute
{
public List AttributeStrings { get; set; }
public List AttributeBytes { get; set; }
public RawAttribute(List attributeStrings, List attributeBytes)
{
AttributeStrings = attributeStrings;
AttributeBytes = attributeBytes;
}
}Of course you should come up with proper names for the properties.
You can adapt this to your other classes e.g:
public LdapAttribute(string name, IEnumerable values, IEnumerable valuesBytes)
{
Name = name;
Values = new ReadOnlyCollection(values.ToList());
ValuesBytes = new ReadOnlyCollection(valuesBytes.ToList());
}You can make the constructor accept
RawAttribute and so you can shorten the initialisation and you can also replace Values && ValuesBytes by just a single instance of RawAttribute.-
ParseSearchResultEntryRanged is way too long and you also have ParseSearchResultEntry, those 2 methods share lots of similarities I would image the first one making use of the later to reduce the code size. If that's not possible you can at least extract the repetitive code in method and just use that.Here is an example of how
ParseSearchResultEntry can look:private Dictionary ParseSearchResultEntry(SearchResultEntry searchResultEntry)
{
Dictionary ldapAttributes = new Dictionary();
if (searchResultEntry?.Attributes?.Values == null)
{
return ldapAttributes;
}
// Collect raw attribute values.
Dictionary rawAttributes = new Dictionary();
foreach (DirectoryAttribute directoryAttribute in searchResultEntry.Attributes.Values)
{
// Prepare collection references.
RawAttribute rawAttribute;
if (!rawAttributes.TryGetValue(directoryAttribute.Name, out rawAttribute))
{
rawAttribute = new RawAttribute(new List(), new List());
rawAttributes.Add(directoryAttribute.Name, rawAttribute);
}
// Collect initial appearance of values.
rawAttribute.AttributeStrings.AddRange((string[])directoryAttribute.GetValues(typeof(string)));
rawAttribute.AttributeBytes.AddRange((byte[][])directoryAttribute.GetValues(typeof(byte[])));
}
// Create LDAP attributes.
foreach (KeyValuePair rawAttribute in rawAttributes)
{
LdapAttribute ldapAttribute = new LdapAttribute(rawAttribute.Key, rawAttribute.Value);
ldapAttributes.Add(rawAttribute.Key, ldapAttribute);
}
return ldapAttributes;
}As you can see I've flipped the
if statement since you only care about your original else body and I'm making use of the class' properties rather than some extra variables. You can apply the same to ParseSearchResultEntryRanged.-
There are some places where you can use the ternary operator instead of
if/elseFunc> searchFunc;
if (PageSize > 0)
{
// Paged search enabled.
searchFunc = SearchPaged;
}
else
{
// Simple search enabled.
searchFunc = SearchSimple;
}
// Assign parser delegate.
Func> parser;
if (RangedRetrieval)
{
parser = searchResultEntry => ParseSearchResultEntryRanged(searchFunc, searchResultEntry);
}
else
{
parser = ParseSearchResultEntry;
}Can be written like this:
// Assign search delegate.
var searchFunc = PageSize > 0
? (Func>) SearchPaged
: SearchSimple;
// Assign parser delegate.
var parser = RangedRetrieval
? (Func>)
(searchResultEntry => ParseSearchResultEntryRanged(searchFunc, searchResultEntry))
: ParseSearchResultEntry;-
To me this isn't clear:
SearchResultEntry rangedSearchResultEntry = null;
foreach (SearchResultEntry entry in searchFunc(searchRequest))
{
if (rangedSearchResultEntry != null)
{
throw new LdapException("Directory returned to many search results during ranged retrieval.");
}
rangedSearchResultEntry = entry;
}
if (rangedSearchResultEntry == null)
{
throw new LdapException("Directory did not return any search results during ranged retrieval.");
}Also your second exception "Directory returned to many search results during ranged retrieval." seems kinda odd, are you excluding the possibility of having a item with value null? This is how I would write it:
```
var searchResu
Code Snippets
private bool rangedRetrieval;
public bool RangedRetrieval
{
get => return rangedRetrieval;
set => rangedRetrieval = value;
}public bool RangedRetrieval { get; set; }public class RawAttribute
{
public List<string> AttributeStrings { get; set; }
public List<byte[]> AttributeBytes { get; set; }
public RawAttribute(List<string> attributeStrings, List<byte[]> attributeBytes)
{
AttributeStrings = attributeStrings;
AttributeBytes = attributeBytes;
}
}public LdapAttribute(string name, IEnumerable<string> values, IEnumerable<byte[]> valuesBytes)
{
Name = name;
Values = new ReadOnlyCollection<string>(values.ToList());
ValuesBytes = new ReadOnlyCollection<byte[]>(valuesBytes.ToList());
}private Dictionary<string, LdapAttribute> ParseSearchResultEntry(SearchResultEntry searchResultEntry)
{
Dictionary<string, LdapAttribute> ldapAttributes = new Dictionary<string, LdapAttribute>();
if (searchResultEntry?.Attributes?.Values == null)
{
return ldapAttributes;
}
// Collect raw attribute values.
Dictionary<string, RawAttribute> rawAttributes = new Dictionary<string, RawAttribute>();
foreach (DirectoryAttribute directoryAttribute in searchResultEntry.Attributes.Values)
{
// Prepare collection references.
RawAttribute rawAttribute;
if (!rawAttributes.TryGetValue(directoryAttribute.Name, out rawAttribute))
{
rawAttribute = new RawAttribute(new List<string>(), new List<byte[]>());
rawAttributes.Add(directoryAttribute.Name, rawAttribute);
}
// Collect initial appearance of values.
rawAttribute.AttributeStrings.AddRange((string[])directoryAttribute.GetValues(typeof(string)));
rawAttribute.AttributeBytes.AddRange((byte[][])directoryAttribute.GetValues(typeof(byte[])));
}
// Create LDAP attributes.
foreach (KeyValuePair<string, RawAttribute> rawAttribute in rawAttributes)
{
LdapAttribute ldapAttribute = new LdapAttribute(rawAttribute.Key, rawAttribute.Value);
ldapAttributes.Add(rawAttribute.Key, ldapAttribute);
}
return ldapAttributes;
}Context
StackExchange Code Review Q#158821, answer score: 3
Revisions (0)
No revisions yet.