On the usage of ‘out’ parameters

The other day, I was discussing with a colleague on whether or not the usage of out parameters is OK. If I’m honest, I immediately cringed as I am not really a fan of said keyword. But first, let’s briefly discuss what on how the ‘out’ parameter keyword works.

In C#, the ‘out’ keyword is used to allow a method to return multiple values of data. This means that the method can return data using the ‘return’ statement and modify values using the ‘out’ keyword. Why did I say modify instead of return when referring to the out statement? Simple, because what the ‘out’ does, is that it receives a pointer to said data structure and then dereferences and applies the value when a new value is assigned. This means that the ‘out’ keyword is introducing the concept of pointers.

OK, the previous paragraph may not make much sense if you do not have any experience with unmanaged languages and pointers. And that’s exactly the main problem with the ‘out’ parameter. It’s introducing pointers without the user’s awareness.

Let’s now talk about the pattern and architecture of said ‘out’ parameter. As we said earlier, the ‘out’ keyword is used in a method to allow it to return multiple values. An ‘out’ parameter gives a guarantee that the value will get initialised by the callee and the callee does not expect the value passed as the out parameter to be initialised.  Let’s see an example:

User GetUser(int id, out string errorMessage)
{
 // User is fetched from database
 // ErrorMessage is set if there was an error fetching the user
}

This may be used as such

string errorMessage;
User user = GetUser(1, out errorMessage);

By the way, C# 7 now allows the out variable to be declared inline, looking something like this:

User user = GetUser(1, out string errorMessage);

This can easily be refactored, so that the message can be returned encapsulated within the same object. It may look something like the below:

class UserWithError
{
 User user {get; set;}
 string ErrorMessage {get; set;}
}

UserWithError GetUser(int id)
{
 // User is fetched from database
 // ErrorMessage is set if there was an error fetching the user
}

Let’s quickly go through the problems with the ‘out’ keyword exposed. Firstly, it’s not easy to discard the return value. With a return value, we can easily call GetUser and ignore the return value. But with the out parameter, we do have to pass a string to capture the error message, even if one does not need the actual error message. Secondly, declaration is a bit more cumbersome since it needs to happen in more than one line (since we need to declare the out parameter). Although this was fixed in C# 7, there are a lot of code-bases which are not running C# 7. Lastly, this prevents from the method to run as “async”.

By the way, ‘out’ raises a Code Quality warning, as defined by Microsoft’s design patterns.

Last thing I want to mention is the use of the ‘out’ keyword in the Try pattern, which returns a bool as a return type, and sets a value using the ‘out’ keyword. This is the only globally accepted pattern which makes use of the ‘out’ keyword.

int amount;
if(Int32.TryParse(amountAsString, out amount))
{
//amountAsString was indeed an integer
}

Long story short, if you want a method to return multiple values, wrap them in a class; don’t use thee ‘out’ keyword.

Can we be a bit more careful on how we use the Internal access modifier?

The other day, I was writing some SharePoint code, and I required a RunWithElevatedPrivileges call. This call is normally accompanied by the creation of a new SPSite and a new SPWeb objects. This is even demonstrated in the RunWithElevatedPrivileges MSDN excerpt, as shown below. What this code does and such, it does not really matter for the sake of this post.


SPSecurity.RunWithElevatedPrivileges(delegate()
{
    using (SPSite site = new SPSite(web.Site.ID))
    {
        // implementation details omitted
    }
});

This is all fine and good, but I’ve noticed that the project that I’m working on already contains loads of RunWithElevatedPrivileges and the accompanying creation of new SPSite and SPWeb; thus I thought that it would be great if I had access to an overload of RunWithElevatedPrivileges that provides a callback with SPSite and SPWeb as parameters rather than creating them myself. So I thought that this is probably offered by SharePoint but a quick look at the public SharePoint API shows that this does not exist.

Then I thought, how is this possible? This is a common use case; somewhere in the SharePoint API, this ought to exist. So, grabbing ILSpy, I’ve reflected the code and gave a quick look. Unsurprisingly, I’ve found the exact overload that I was looking for. Though, for some weird reason, it’s set to Internal, rather than public. Hold on a minute, why is this kind of API not public? This is not some kind of abstraction; it’s API that should be readily available for the developer.


// Microsoft.SharePoint.SPSecurity
internal static void RunWithElevatedSiteAndWeb(SPWeb originalWeb, SPSecurity.CodeToRunWithElevatedSite secureCode)
{
    if (originalWeb.CurrentUser != null && originalWeb.CurrentUser.ID == 1073741823 && !originalWeb.Site.HasAppPrincipalContext)
    {
        secureCode(originalWeb.Site, originalWeb);
        return;
    }
    SPSecurity.RunWithElevatedPrivileges(delegate
    {
        using (SPSite sPSite = new SPSite(originalWeb.Site.ID, originalWeb.Site.Zone))
        {
            using (SPWeb sPWeb = sPSite.OpenWeb(originalWeb.ID))
            {
                secureCode(sPSite, sPWeb);
            }
        }
    });
}

This made me think: can we be a bit more careful on how we use the Internal access modifier? I mean, I understand that portions of the code should be private, since such code will be only used in the same class to simplify the underlying code. But, API that are clearly useful by developers having a LOT of internal methods is a big no for me. It is clearly not adding business value to the API, just frustration to the end developer since he needs to re-implement (or copy) the same implementation in his solution.

Obviously, I am not saying that ALL Internal methods are badly designed; if this was the case, it would not exist at all. I’m saying that API developers should think twice before limiting API to internal, which can clearly be used 3rd party developers. Private methods are OK, but internal methods, I think one needs to be a bit more careful on how this is used.

Or..maybe this is just one of the many, many quirks of the SharePoint API.