Can anyone tell me why the maintainability index is only 40 for this code?

Question

I can't figure out why the maintainability index (as calculated in Visual Studio) for this method is only 40, I literally have to remove almost all the lines except the first two to get above 60:

    public void getNewPasswordDetails(Int32 newPasswordId)
    {

        int UserId = HttpContext.Current.User.Identity.GetUserId().ToInt();
        bool userIsAdmin = HttpContext.Current.User.IsInRole("Administrator");

        //get a list of userIds that have UserPassword records for this password
        var UserIDList = DatabaseContext.UserPasswords.Where(up => up.PasswordId == newPasswordId).Select(up => up.Id).ToList();

        //Retrive the password -if the user has access
        Password newPassword = DatabaseContext.Passwords
                                                        .Include("Creator")
                                                        .Where(pass => !pass.Deleted
                                                        && (
                                                            (UserIDList.Contains(UserId))
                                                         || (userIsAdmin && ApplicationSettings.Default.AdminsHaveAccessToAllPasswords)
                                                         || pass.Creator_Id == UserId)
                                                            )
                                                            .Include(p => p.Parent_UserPasswords.Select(up => up.UserPasswordUser))
                                                            .SingleOrDefault(p => p.PasswordId == newPasswordId);



        if (newPassword != null)
        {
            //map new password to display view model
            AutoMapper.Mapper.CreateMap<Password, PasswordItem>();
            PasswordItem returnPasswordViewItem = AutoMapper.Mapper.Map<PasswordItem>(newPassword);

            //generate a string based view of the new category
            string passwordPartialView = RenderViewContent.RenderViewToString("Password", "_PasswordItem", returnPasswordViewItem);

            //broadcast the new password details
            PushNotifications.sendAddedPasswordDetails(passwordPartialView, returnPasswordViewItem.Parent_CategoryId, returnPasswordViewItem.PasswordId);
        }
        else
        {
            //we dont have access any more, so tell UI to remove the password
            PushNotifications.sendRemovePasswordAccess(new PasswordDelete()
                                                                { 
                                                                     PasswordId = newPasswordId
                                                                });
        }

    }

[This](http://blogs.msdn.com/b/codeanalysis/archive/2007/11/20/maintainability-index-range-and-meaning.aspx) is an older post, but it shows how the maintainability index is calculated. It should give you an idea of why your index is lower than your target. — hunch_hunch, Nov 19 '14 at 18:27

ΩmegaMan · Answer 1 · 2014-11-19T20:03:41.570

The more complex code is, the harder it is to maintain. Right? So let us look at a complex section of code which was presented. I will examine it as if I was a developer looking at the code for the first time:

DatabaseContext.Passwords
               .Include("Creator")
               .Where(pass => !pass.Deleted
                          && ((UserIDList.Contains(UserId))     // Why Check #1
                               || (
                                   userIsAdmin                // Why Check #2
                                   &&                        // Why Check #3
                                   ApplicationSettings.Default.AdminsHaveAccessToAllPasswords
                                  )
                              || pass.Creator_Id == UserId) 
                             )
               .Include(p => p.Parent_UserPasswords.Select(up => up.UserPasswordUser))
               .SingleOrDefault(p => p.PasswordId == newPasswordId);

Before entering the loop one already knows these state facts:

(UserIDList.Contains(UserId) as Why Check #1
userIsAdmin as why Check #2
(userIsAdmin && ApplicationSettings.Default.AdminsHaveAccessToAllPasswords) as why check #3

Yet the developer has relegated those checks to occur for every password in Passwords. Why? Isn't there a better way of expressing that ?

Complexity occurs due to the application (coding) of the program logic, for each logical fork in the determination of the business logic directly adds to the complexity of the code and subsequently future maintainability; hence the rating received.

Of course one has certain complexities in code just by its nature, that is going to happen and should be expected. The question is, can that complexity be minimized to the point where maintenance of the code can be better achieved.

As a suggestion on how you could improve it you could do the line `var overridePassword = \\everything that was in the parenthesis;` before the current line then change the `Where` would simplify to be `.Where(pass => !pass.Deleted && (overridePassword || pass.Creator_Id == UserId))` — Scott Chamberlain, Nov 19 '14 at 19:23
OK, I understand, but making that one bit of code less complex does nothing to the maintainability index. — binks, Nov 21 '14 at 00:41

Can anyone tell me why the maintainability index is only 40 for this code?

1 Answers1