Active Directory Provider

Topics: Developer Forum
Developer
Jan 28, 2007 at 3:50 AM
Reinherd, first of all, you provider looks great, I can't wait to get to a domain to try it out. However, I've been looking thru the code and I found few things I don't consider optimal:

- you are catching (and swallowing) exceptions a lot, seems to me, it is not not necessary in many cases.

- the ActiveDirectoryEntry should be replaced by a type extension and/or format extension of the DirectoryEntry, it does not add any other value except of the user-friendly entry type description.

- there is still lot of inconsistency in your field, variable and parameter naming; please follow the .NET & PSCX guidelines

- I dont understand why you have the DirectoryEntryClass enumeration declared twice

- you are switch()ing on that DirectoryEntryClass enum way too often. it seems to me as a perfect candidate for replacing with a base class which would abstract the differences away.

- you are using ArrayLists. I consider that class obsolete, given that List<T> has a richer interface, is more performant and is a better choice even if you want to use it as List<Object>. However, given the number of typecasts I see next to each ArrayList, I assume it's not that case either.

- the NameTranslateClass was obsolete last time I checked, please translate the names by other means

- you should use the resources for localizable strings (what if we are going to do a german or czech version in the future? :))

- i'd welcome removing the ActiveDs interop assembly dependency, and moving the required interfaces and enums to PscxCore\Interop (or something). you'd be able to rename the enums to match .NET style, and use them in your interfaces then.

- last but not least, please watch out for typos, I saw lots of them...
Developer
Jan 28, 2007 at 3:53 AM
oh, i'm sorry for screwing up your name, its not easy to switch between english and german for me...
Developer
Jan 28, 2007 at 4:04 AM
- case insensitive string comparison should be done using string.Equals(a, b, StringComparison.OrdinalCaseInsensitive), instead of calling ToLower()/ToUpper()

- you should dispose of any temporary DirectoryEntries, DirectorySearchers and other IDisposable objects

- determining whether an object is of some type should not be done using GetType().FullName comparison. you should just use the "is" c# operator
Developer
Jan 28, 2007 at 4:14 AM
WRT the last point: in my opinion, the most clear way of determining the object type and doing something based on it, is the following style:
IInterface1 i1 = obj as IInterface1;
IInterface2 i2 = obj as IInterface2;
 
if (i1 != null)
{
}
 
if (i2 != null)
{
}

WRT ActiveDirectoryDriveInfo: I believe the type should not be mutable. Particulary, you should not be able to change the drives root after creation, right? Also, why do you need to store both the CN and the DN? Wouldn't a single form be sufficient?
Developer
Jan 28, 2007 at 9:31 AM
Thanks for the suggestions and criticisms. I will change it.