I am designing a test for an existing static method:
public static boolean hasPermissions(String username, int pageid) {
PermissionsService s = new PermissionsService();
int[] pages = s.getPages(username);
for (int i = 0; i < pages.length; i++) {
if (pages[i] == pageid)
return true;
}
return false;
}
and I want to manipulate the response coming back from the service, and proposed adding another parameter, hasPermissions( .. , PermissionsService ps)
. However, as there are hundreds of calls to PermissionEngine.hasPermissions
across the application, the other teams (who are also using the code) are objecting to this change.
Am I right to think that this is how tests are designs and our static method is tech debt because it wasn’t designed with TDD in mind?
1
Am I right to think that this is how tests are designs and our static method is tech debt because it wasn’t designed with TDD in mind?
Arguably.
Personally, I don’t think that you should change your code to make it more testable. That’s a fairly common viewpoint, as the goal of software is to solve some problem. Testing doesn’t directly contribute to that, and making the function more complex is clearly worse at solving the problem.
That said, when it’s hard to test your code, it’ll be hard to change your code. Since change is pretty inevitable, making your code flexible will help you over the longer term. Sometimes that long term benefit isn’t worth it, but it’s rare.
As for this particular example, I’d be a little hesitant to inject the service. Having a simple abstraction for hasPermissions
is useful. A better spot for the abstraction would be at your own consumer. It could take an interface or delegate/function-object that calls this static function in production. You could then mock that in your tests, without influencing all of the other consumers.
Refactoring this inflexible implementation can then happen over the longer term. (Ideally, removing the wildly inefficient linear search)
2
Am I right to think that this is how tests are designs and our static method is tech debt because it wasn’t designed with TDD in mind?
Technical debt doesn’t (in its original form, anyway) mean bad code, or code that we don’t like. It was specifically referring to incorrect alignment between the real domain and the design in code.
if we failed to make our program align with what we then understood to be the proper way to think about our financial objects, then we were gonna continually stumble over that disagreement and that would slow us down
This is just (take your pick):
- bad code
- code not designed with the affordances you need right now
other teams (who are also using the code) are objecting to this change.
And they are right to do so. Making a backwards incompatible change is an awful idea.
What you should instead do is make the change in a backwards compatible way, then (if necessary) deprecate the old way of doing things and negotiate a schedule to get everybody on board with the new approach.
The rigorous way to do it is to create a function with a new name that has the design you want, and then modify the old function to invoke the new function.
There are at least two ways you could do that here. One is, as you suggested, creating a function that takes the service as an argument
public static boolean hasPermissions(String username, int pageid) {
PermissionsService s = new PermissionsService();
return hasPermissionsV2(username, pageid, s);
}
public static boolean hasPermissionsV2(String username, int pageid, PermissionsService s) {
int[] pages = s.getPages(username);
for (int i = 0; i < pages.length; i++) {
if (pages[i] == pageid)
return true;
}
return false;
}
A different approach, which might be better for your situation, is to separate the code that manipulates values from the spooky side effects
public static boolean hasPermissions(String username, int pageid) {
PermissionsService s = new PermissionsService();
int[] pages = s.getPages(username);
return hasPermissionsV3(pages, pageid);
}
public static boolean hasPermissionsV3(int [] pages, pageid) {
for (int i = 0; i < pages.length; i++) {
if (pages[i] == pageid)
return true;
}
return false;
}
I suggest you review Spec-ulation; Rich Hickey’s 2016 talk has a lot of interesting things to say about names, and the advantages of not changing the meanings of names.