3
\$\begingroup\$

I hope this is the right place for this. I basically wrote the below script for a SharePoint 2013 where I needed to get the department of the currently logged in user.

I was finally able to get it working, but I am not happy with the structure at all:

    // globals
    var currentUser;
    var currentUserName;
    var property = "Department";
    // ctx
    var clientContext = new SP.ClientContext.get_current();
    var oWeb = clientContext.get_web();
    currentUser = oWeb.get_currentUser();
    clientContext.load(currentUser);    

    GetCurrentUserProperty();

    // This function first gets the current user's firstname.lastname username (e.g. Joe.Bloggs).
    // If this is successful, it calls the LoadPropertyForUser function, which will retrieve the user's
    // property which was defined in the global "property" variable.
    function GetCurrentUserProperty(){
        clientContext.executeQueryAsync(onQueryUserSuccess, onQueryUserFail);
    }
    function onQueryUserSuccess() {
        // If the query is successful, extract the first.last username and then call LoadPropertyForUser
        window.currentUserName= currentUser.get_loginName().split("\\")[1]; 
        LoadPropertyForUser(window.currentUserName);
    }
    function onQueryUserFail(sender, args) {
        alert('Failed to retrieve user name');
    }

    function LoadPropertyForUser(userName){                 
        //Get Instance of People Manager Class
        var peopleManager = new SP.UserProfiles.PeopleManager(clientContext);
        //If you are on On-Premise:
        var targetUser = "DOMAIN1\\" + userName;
        //Create new instance of UserProfileProperty
        departmentProperty = peopleManager.getUserProfilePropertyFor(targetUser, window.property)

        //Execute the Query. (No load method necessary)
        clientContext.executeQueryAsync(onPropertyLoadSuccess, onPropertyLoadFail);
    }
    function onPropertyLoadSuccess() {    
        var messageText = window.property + " is " + departmentProperty .get_value();
        alert(messageText);
    }
    function onPropertyLoadFail(sender, args) {
        alert("Error: " + args.get_message());
    }

</script>

As you can see, the GetCurretUserProperty method actually continues on in the onQueryUserSuccess function by calling LoadPropertyForUser. While this does work, I felt that this sort of layout would make a lot more sense :

function Main (property) {
    var property = "Department"
    var username = GetCurrentUserName();
    LoadPropertyForUser(username, property);
}

But I was having trouble with this because whenever I started passing variables from GetCurrentUserProperty to it's success method, I would get errors about loginName not being properly loaded.

If anyone could give me some insight as to how this would be structured I will be very grateful. I also welcome and other feedback people may have - thank you!

\$\endgroup\$
2
\$\begingroup\$

Overall, your code's fine. The only thing that really jumps out to me is that it clutters the global namespace with some variables; I'm not sure if that was intentional (such as if other code is then referencing those variables), but I'd try to avoid that if you can.

I understand your unease with the way the code is written, so I'm going to use the rest of this answer to (hopefully) address that concern, and then I'm going to offer some suggestions for making your code more concise and arguably more readable ...but that's admittedly a matter of personal preference.

Patterns for Asynchronous JavaScript

After working with any programming language for a while you'll start to recognize certain patterns for the "right" way to accomplish something. This is especially true of asynchronous JavaScript, which doesn't immediately mesh with the single-threaded synchronous coding mindset we programmers usually start with.

The biggest eye opener for me was to recognize that as soon as you've made your code dependent on the execution of an asynchronous function call, you should stop adding code to the current function, because when the asynchronous function actually executes, the current function will no longer be within the execution context.

// Anti-pattern: What NOT to do
function doSomething(){
    doSomethingAsync();
    doSomethingElse(); // Usually a logic error; doSomethingAsync() hasn't finished
}

Note that this also explains why you can't get a return value from an asynchronous function call.

// Another Anti-pattern
function doSomething(){
    var result = doSomethingAsync();
    doSomethingElse(result); // This is an even worse logic error!
}

The pattern for dealing with asynchronous dependencies is to move your dependent logic forward into the execution chain. This usually means passing callback functions to your asynchronous functions.

You're handing off the responsibility of executing the callback function to the new function ("Hey, you're in charge now; do this other thing once you're done.")

function doSomething(){
    doSomethingAsync(doSomethingElse);
}

Of course, to make the code more readable, you could mess with the structure of the parameters, such as by using objects with named properties...

function doSomething(){
    doSomethingAsync({whenDone: doSomethingElse});
}

function doSomethingAsync(options){
     // insert JSOM code here
     // ...
     clientContext.executeQueryAsync(
         function(){ // on success
             options.whenDone(); // if doSomethingElse wants parameters, could provide them here
         },
         function(){ // on error
         }
     );
}

Using Function Expressions

Now with all that being said, sometimes having to define multiple functions just to insert logic farther down the execution chain actually hinders readability.

A function that's only ever referenced once, and only because you need to pass it as a callback, isn't doing anything to keep your code DRY (Don't Repeat Yourself), and can make it more tedious to debug or map out the chain of execution events. For these reasons I'll often use inline function expressions instead of defined functions for callbacks.

Inline function expressions can replace your globally defined callback functions, such as onQueryUserSuccess, and onLoadPropertyFail.

I mentioned at the beginning of this answer that I dislike polluting the global namespace with variables. Using function expressions, I can refer to variables defined in the containing function without needing to attach them to the window object. (Read about closures in JavaScript to understand how this works.)

This is definitely a matter of personal preference, but I prefer the code reformatted into a single function with nested function expressions for the executeQueryAsync callbacks, like so:

function loadUserDepartment(){
    var property = "Department";
    var clientContext = new SP.ClientContext.get_current();
    var currentUser = clientContext.get_web().get_currentUser();
    clientContext.load(currentUser); 
    clientContext.executeQueryAsync(
        function(){ // successfully retrieved current user
            var currentUserName = currentUser.get_loginName().split("\\")[1];
            var peopleManager = new SP.UserProfiles.PeopleManager(clientContext);
            // retrieve UserProfileProperty...
            var targetUser = "DOMAIN1\\" + userName;
            var departmentProperty = peopleManager.getUserProfilePropertyFor(targetUser, property)
            clientContext.executeQueryAsync(
                function() { // successfully retrieved property
                    alert(property + " is " + departmentProperty.get_value());
                },function()(sender, args) { alert("Error: " + args.get_message()); }
            );
        }, 
        function(sender,args){ alert('Failed to retrieve user name.'); }
     );
}

Now I can understand why the above code might give you heartburn. It's essentially a nested decision tree instead of a sequence of straightforward (and readable) function calls. But it's much easier for me to trace the execution chain and understand what the code is doing at a glance, and there are no variables attached to the window object (...well, except for the containing loadUserDepartment function, but you could avoid even that by converting it to an immediately executing function expression (IEFE)).

Notice that within the scope of their containing functions, the calls to clientContext.executeQueryAsync are always the last lines of code to be executed, reflecting the pattern I discussed way back at the beginning of this post.

\$\endgroup\$

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.