6
\$\begingroup\$

I am writing automated tests for a web app and at one point I need to make sure that a user can only engage in activities allowed by a set permission. I have written all the standalone methods that perform the required tests and now I need to group all of them together and test. I came up with something that looks like this:

public boolean hasPermissions(String permissions, File file) {

    if ("Read Only".equals(permissions)) {

        return !canCreateNewFolder() && !canAddFiles() && !canAddLink() && canCopyMultipleItems() && !canMoveMultipleItems()
                && !canDeleteMultipleItems() && canDownloadMultipleItems() && canDownload(file) && canViewDetails(file)
                && !canEdit(file) && canCopy(file) && !canMove(file) && !canHide(file) && !canDelete(file);

    } else if ("Read and Add Only".equals(permissions)) {

        return canCreateNewFolder() && canAddFiles() && canAddLink() && canCopyMultipleItems() && !canMoveMultipleItems()
                && !canDeleteMultipleItems() && canDownloadMultipleItems() && canDownload(file) && canViewDetails(file)
                && !canEdit(file) && canCopy(file) && !canMove(file) && !canHide(file) && !canDelete(file);

    } else if ("Full Access".equals(permissions)) {

        return canCreateNewFolder() && canAddFiles() && canAddLink() && canCopyMultipleItems() && canMoveMultipleItems()
                && canDeleteMultipleItems() && canDownloadMultipleItems() && canDownload(file) && canViewDetails(file) && canEdit(file)
                && canCopy(file) && canMove(file) && canHide(file) && canDelete(file);

    } else {
        throw new UnsupportedOperationException("Unsupposrted permission");
    }

}

While this solution works with no issues, it somehow doesn't look right.

So just wanted to ask for another opinion on this design.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please alter your question to contain the full list of conditions; answers which may work for 3 boolean conditions might not work for 20 \$\endgroup\$
    – Pimgd
    Aug 8, 2016 at 8:32

1 Answer 1

10
\$\begingroup\$

So, thing is, it's GREAT that you made the individual tests. Imagine this method with all those tests inlined. That'd be... really bad. So that part is good. But like you say, something is off.

Now, one way you could look at this is that you don't really need to test everything. That depends on what an implementation means, but someone has read rights if they can read a file. Whether or not they can write... it doesn't matter. But, that depends on your business requirements, so such a simplification may not apply.

Another thing you could do is apply semantic grouping. Combine checks that are in the same category.

There seems to be 3 groups:

Read access, which is the ability to read any file (canCopyMultipleItems, canDownloadMultipleItems, canDownload, canViewDetails, canCopy).

Create+Read access, which is read + append to directory. (canCreateNewFolder, canAddLink)

Write access, which is read + create + write. (canMoveMultipleItems, canDeleteMultipleItems, canEdit, canMove, canHide, canDelete).

If you are only interested in verifying that someone has "at least" these rights, this is easy, you can make three groups of checks (each of them including the previous) and return the appropriate value.

If, however, you're only allowed to have the exact matching set of rights, then the check becomes harder, I'd dare say that what you have right now is the fastest code you could have; any faster is going to be faster because of a redesign.

From a readability perspective, all I'd do is this:

Wrap each grouping we just mentioned in two separate functions:

hasAny<group>Right and hasAll<group>Rights.

Then you can use them like so:

public boolean hasPermissions(String permissions, File file) {

    if ("Read Only".equals(permissions)) {
        return hasAllReadRights(file) && !hasAnyCreateRight(file) && !hasAnyWriteRight(file);
    } else if ("Read and Add Only".equals(permissions)) {
        return hasAllReadRights(file) && hasAllCreateRights(file) && !hasAnyWriteRight(file);
    } else if ("Full Access".equals(permissions)) {
        return hasAllReadRights(file) && hasAllCreateRights(file) && hasAllWriteRights(file);
    } else {
        throw new UnsupportedOperationException("Unsupposrted permission");
    }
}

Perhaps even name "create" "add" instead.


That all said, you... clearly have a design issue going on. A string goes in, and based on that you're doing checks? Huh?

First, stringly typed code should be replaced with Enums. Second, having to check every single time if the user has a set of general rights is ... weird. These should be migrated to a different place; then all you're interested in is if the user has generic read rights, + the few things that are file specific. If instead you're relying on class variables in these "generic" checks (like canDownloadMultipleItems), then... well... then I still think it's worth splitting these checks up into "generic" and "file-specific" checks; if you're ever going to be looping over a directory, you'll want to be checking the generic parts only once, and not all the time.


Also, there's a typo in your exception. "Unsupposrted permission" -> "Unsupported permission"

\$\endgroup\$
5
  • \$\begingroup\$ Wow! Thanks a lot for your elaborate answer! It is a great help. I will follow your suggestion to group the permissions. I still want to test each assertion as I want to ensure that none of the "generic" ones has changed. \$\endgroup\$
    – Eugene S
    Aug 8, 2016 at 9:07
  • \$\begingroup\$ @EugeneS What I'm saying is, if you're checking the permissions for all the files in a directory, you could check the generic permissions once, then loop over the files, checking the file permissions for each file, rather than checking all the permissions for each file. \$\endgroup\$
    – Pimgd
    Aug 8, 2016 at 9:12
  • \$\begingroup\$ Oh, I that's not real files in a directory I am dealing with here. These are files that can be added and manipulated by users in a web app. \$\endgroup\$
    – Eugene S
    Aug 8, 2016 at 9:17
  • \$\begingroup\$ @EugeneS a virtual image doesn't contain real files either, yet you can still make directories and read files and write files and all that on a virtual machine. But I guess that use-case could be non-existent. \$\endgroup\$
    – Pimgd
    Aug 8, 2016 at 9:45
  • \$\begingroup\$ Yes, of course. What I meant to say is that in this case I do not need to check each file but just the functionality that is available to a user currently logged into the application. \$\endgroup\$
    – Eugene S
    Aug 8, 2016 at 14:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.