Retrieving the Currently Logged in User's Department

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 messag

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 h

StackExchange Code Review Q#130041, answer score: 2