Determining complex security and permissions

I've been put on a project that has a me开发者_运维问答ssy class that writes buttons to a page. The app is a document manager and has a popup list of buttons, such as download, email, and print. Depending on the user's roles, and the state of the document, different buttons are displayed.

Among other WTFs is something like this:

bool showEditButton = document.docTypeId == documentEnum.docType.text && 
( document.statusId == documentEnum.docStatus.Editable || (user.UserStatus == userEnum.Status.SuperUser) || ( user.UserID == document.CreatedByUserId ) )

And so on and so forth until I can't figure out what's going on.

I don't know if this is just a side effect of a deeper architectural flaw, or if there's a good method to deal with checking a mixture of permissions and status values. Should I just put all of these crazy conditions in a method and just forget about it? That doesn't benefit the next programmer to inherit the project though.

---

It's just a bunch of boolean logic in your example, but the readability could be cleaned up with a Compose Method refactoring. If you had a class that accepted a document and the current user principal, then you could have something like:

public class DocumentPermissions
{
    private Document document;
    private User user;

    public DocumentPermissions(Document doc, User currentUser)
    {
        document = doc;
        user = currentUser;
    }

    public bool ShouldShowEditButton()
    {
        if(!IsTextDocument())
        {
            return false;
        }
        return IsSuperUser() || IsDocumentOwner() ||  DocumentIsEditable();
    }

    private bool IsTextDocument()
    {
        return document.docTypeId == documentEnum.docType.text;
    }

    private bool IsSuperUser()
    {
        return user.UserStatus == userEnum.Status.SuperUser;
    }

    private bool IsDocumentOwner()
    {
        return user.UserID == document.CreatedByUserId ;
    }

    private bool DocumentIsEditable()
    {
        return document.statusId == documentEnum.docStatus.Editable ;
    }
}

Obviously this is a lot of code so I hope you can make reuse of many of the private methods.

---

Alternatively you could use:

bool showEditButton = (document.statusId == documentEnum.docStatus.Editable); //show if Editable..
showEditButton |= (user.UserStatus == userEnum.Status.SuperUser); //or a superuser or
showEditButton |= (user.UserID == document.CreatedByUserId); //the Creator
showEditButton &= (document.docTypeId == documentEnum.docType.text); //and a text Doc

Although, I prefer Ryan's answer, I will throw this out for another way that is at least marginally more readable and give a better spot for some comments.

---

You own it for the time being; if you've been assigned to refactor it, refactor it. If you have other more pressing issues, deal with them, but you should take your spare time to refactor it if you can (Don't do TOO good a job, they might make you permanent owner). Regarding your other questions, security etc., not enough info.

http://en.wikipedia.org/wiki/Refactoring

---

To be honest, the example code doesn't look too bad. I've certainly seen a lot worse.

It's quite readable and has no "magic strings" or "magic numbers". I'm sure you can find more pressing opportunities for a clean-up.