2
\$\begingroup\$

I'm working on a rather large SharePoint project that's going to contain helper classes for a variety of item levels (sites, webs, lists, items, etc').

When writing code I'm expecting that the same site and authentication will span across multiple contexts (as in you might use a list helper, and then an item helper with the same credentials). For this reason I've built a credentials class to hold onto those.

Also since site names are going to be used in every helper I've built a "base helper class to hold the information.

Here are my current classes:

My authentication class

/// <summary>
/// SharePoint Class for doing quick authentication tests with a SharePoint Online Server
/// </summary>
public class SPAuthentication
{
    /// <summary>
    /// Initialize authentication class with connection information
    /// </summary>
    /// <param name="username">The <paramref name="username"/> (@domain or @onmicrosoft.com) for connecting to SharePoint Online</param>
    /// <param name="password"></param>
    /// <exception cref="CryptographicException">An error occurred when encrypting the parameter <paramref name="password"/> into a secure string</exception>
    /// <exception cref="ObjectDisposedException">This secure string has already been disposed.</exception>
    /// <exception cref="ArgumentOutOfRangeException">Performing this operation would make the length of this secure string greater than 65536 characters.</exception>
    public SPAuthentication(string username, string password)
    {
        var securePassword = new SecureString();

        // use password input to create a secure string for use with SharePoint Online
        foreach (var c in password.ToCharArray()) securePassword.AppendChar(c);

        Credentials = new SharePointOnlineCredentials(username, securePassword);
    }

    /// <summary>
    /// Container to hold SharePoint credentials after instantiating class. This prevents the user having to repeatably fetch the user/pass and recreate a credentials class
    /// </summary>
    public readonly SharePointOnlineCredentials Credentials;
}

My base helper class

/// <summary>
/// A base helper class for use with other SharePoint related helper.
/// </summary>
public class SPBaseHelper
{
    /// <summary>
    /// Backing field for SiteUrl property
    /// </summary>
    private string _spSiteUrl;

    /// <summary>
    /// Property for storing the SharePoint site to be used for operations
    /// </summary>
    public string SiteUrl
    {
        get { return _spSiteUrl; }
        set
        {
            _spSiteUrl = value;
            _spSiteUrl = _spSiteUrl?.Replace(@"\", @"/");
        }
    }

    /// <summary>
    /// Current SHarePoint authentication information.
    /// </summary>
    internal SharePointOnlineCredentials SPCredentials;

}

The foundation of my first helper class (lists)

/// <summary>
/// A helper class for use with SharePoint lists using the client object model
/// </summary>
public class SPListHelper : SPBaseHelper
{
    /// <summary>
    /// Instantiate new helper class with
    /// </summary>
    /// <param name="auth">SharePoint Class for doing quick authentication tests with a SharePoint Online Server</param>
    public SPListHelper(SPAuthentication auth)
    {
        SPCredentials = auth.Credentials;
    }

    /// <summary>
    /// Perform a test connection to a SharePoint Online Site
    /// </summary>
    /// <returns>Returns the site name that the class was opened with</returns>
    public string TestConnect()
    {
        if (SiteUrl == null) return "No site specified.";

        using (var ctx = new ClientContext(SiteUrl))
        {
            ctx.Credentials = SPCredentials;

            var web = ctx.Web;

            ctx.Load(web);
            ctx.ExecuteQuery();

            return web.Title;
        }
    }
}

Is this going to be too difficult for those that follow? Is there a better approach I could change to this early on into a project, what general improvements could I make?

\$\endgroup\$
  • \$\begingroup\$ Can we get more information about how you'll use this code? Typically credentials go in a configure file or database. \$\endgroup\$ – RubberDuck Aug 2 '15 at 10:22
  • \$\begingroup\$ @RubberDuck The configuration will vary depending on the application using the classes (I'm intending on having a WPF application and a console application consuming these classes). I'm using JSON to store the configuration which the front end application will read and pass to the classes (since this JSON varies in each use case). \$\endgroup\$ – Michael A Aug 2 '15 at 10:54
2
\$\begingroup\$

Elvis is in the building!

public string SiteUrl
{
    get { return _spSiteUrl; }
    set
    {
        _spSiteUrl = value;
        _spSiteUrl = _spSiteUrl?.Replace(@"\", @"/");
    }
}

I like that you're using the null conditional operator (Elvis), but I'm wondering if it's really necessary here. Neither your property or your backing field are nullable here, so if I recall correctly, there's no way for value to be null. Therefore, there's no reason to null check it and this could be simplified down a bit.

public string SiteUrl
{
    get { return _spSiteUrl; }
    set
    {
        _spSiteUrl = value.Replace(@"\", @"/");
    }
}

If you really like that safety net, then call it directly on value.

_spSiteUrl = value?.Replace(@"\", @"/");

I was totally wrong about that. String is a reference type, hence nullable. I'm a derpy duck sometimes, but I like your use of the Elvis operator even more now.


This ctor should also be taking in a SecureSting IMHO.

public SPAuthentication(string username, string password)
{
    var securePassword = new SecureString();

    // use password input to create a secure string for use with SharePoint Online
    foreach (var c in password.ToCharArray()) securePassword.AppendChar(c);

    Credentials = new SharePointOnlineCredentials(username, securePassword);
}

I love that you're using SecureString, I really do, but the idea is to transform user input into one as it's entered, not at a later date. Storing it as a string at all, no matter how temporarily, defeats the purpose entirely. If you're using , then the password box handles this for you, in the console you just append one character at a time as the user types.


I like your code more or less. It's neat and tidy, the classes are small and have well defined functionality. I just want to clarify a point @dreza made in their answer.

Rather than make a property public readonly. Consider making it a public property with private setters.

They were talking about this code.

/// <summary>
/// Container to hold SharePoint credentials after instantiating class. This prevents the user having to repeatably fetch the user/pass and recreate a credentials class
/// </summary>
public readonly SharePointOnlineCredentials Credentials;

Which is actually a public readonly field and you should use public properties instead of fields. You're using C# 6.0 though, so there's a better way than using a private setter.

public SharePointOnlineCredentials Credentials { get; }

Yup. That's it. Beautiful aint it? If you're targeting an earlier version, you'd have to write it like this to get a truly read-only property.

private readonly SharePointOnlineCredentials _creds;
public SharePointOnlineCredentials Credentials
{
    get { return _creds; }
}

If you're curious as to why I recommend the read-only version over the private setter, it's because read-only is exactly that, read-only. It can only get set once. Private setter only protects you from outside code changing the value, it could still be modified inside of the class it's defined in.

\$\endgroup\$
  • \$\begingroup\$ A question. When you say the SiteUrl property is not nullable, I always thought a string was nullable in c#? Is there something different about properties? \$\endgroup\$ – dreza Aug 2 '15 at 21:31
  • \$\begingroup\$ I could be wrong @dreza. I was working from memory. \$\endgroup\$ – RubberDuck Aug 2 '15 at 21:32
  • \$\begingroup\$ @dreza you were right about string being nullable. \$\endgroup\$ – RubberDuck Aug 3 '15 at 1:51
1
\$\begingroup\$

There will be other answers that will most likely provide more helpful feedback. But my initial thoughts on reading this are:

  1. Use full names for variables. When I saw SPAuthentication I didn't know what SP stood for until I saw the comments. So perhaps a better name would have been SharePointAuthentication. That goes or all other SP prefixes. If I thought that, odds are that others would to.

  2. Rather than make a property public readonly. Consider making it a public property with private setters. i.e.

    public SharePointOnlineCredentials Credentials { get; private set; }
    

This will help provide a better layer of abstraction whereby users of Credentials itself does not have access directly to the implementation details of the variable. For example, it could be backed by a lazy property, by a method, by an internal variable.

\$\endgroup\$
  • 2
    \$\begingroup\$ OP is using C# 6.0. Readonly auto-props are preferable to private setters. \$\endgroup\$ – RubberDuck Aug 2 '15 at 10:36
  • \$\begingroup\$ The class library project is already labelled with the full name qualification along with the namespaces, I'm mostly using SP here to avoid naming conflicts. Do you still think the full name should apply in this case? I typically use the acronym over full names as they are used by the object model and I feel reading my code becomes faster when I know what's bespoke, and what's in the libraries by Microsoft. \$\endgroup\$ – Michael A Aug 2 '15 at 10:57
  • \$\begingroup\$ @RubberDuck Wow, how did you know it was c# 6? \$\endgroup\$ – dreza Aug 2 '15 at 20:24
  • \$\begingroup\$ @Codingo I don't know. I guess I would use the namespace as the prefix so SharePoint.Authentication in that case. It's just that some classes are named with SharePoint i.e. SharePointOnlineCredentials so seems wise to keep following that convention?? \$\endgroup\$ – dreza Aug 2 '15 at 20:25
  • \$\begingroup\$ @dreza the null conditional operator (the Elvis operator) was just introduced with C#6.0. That's how I could tell. \$\endgroup\$ – RubberDuck Aug 2 '15 at 20:38

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.