Should a function that simply passes an argument to another function do type checking on that argument?

Question

Given the following two (simple/random) PHP methods in the same class:

/*
 * @param $a An int to play with
 * @param $b An int to play with
 * @param $c An int to play with
 *
 * @throws InvalidArgumentException when either $a, $b, or $c are not an int
 *
 * @return A new int
 */
public function1($a, $b, $c) {
    if(!is_int($a)) throw new InvalidArgumentException('$a must be an int');
    if(!is_int($b)) throw new InvalidArgumentException('$b must be an int');

    $x = $a * $b;

    $y = $this->function2($c);

    return $x - $y;
}

/*
 * @param $c An int to play with
 *
 * @throws InvalidArgumentException when $c is not an int
 *
 * @return A new int
 */
private function2($c) {
    if(!is_int($c)) throw new InvalidArgumentException('$c must be an int');

   return $c + 1;
}

Two part-question:

• Should function1() also check the argument type for $c?
• When testing, say using PHPUnit, is it enough to test a bad argument for function2, or should I also write a second test to test passing a bad $c into function1?

It is possible that function2() could be called by other functions besides function1().

On the one hand, I think that a function should check everything given to it. On the other hand, I feel like it could lead to a lot of duplicate and (while not with these specific functions) costly code.

Answer 1

It doesn't really matter if function1 checks parameter c or not, it's largely a stylistic choice. Some people like to perform ALL checking at the start of a function because it means that the function can be aborted as soon as possible, without any unnecessary processing taking place. If there was significant processing before the call to function2 then there would be more reason for the check, but as it stands the important thing is that the parameter is checked before it is actually used.

As far as your second question goes, YES you should test passing a bad parameter to function1. Personally, I don't think you should be testing passing a bad parameter to function2. In fact, from a unit test perspective, you shouldn't even know that function2 exists.

The public methods of a class determine the public interface and hence the testable api for the class. In other words, if I am a client for your class, I can call any of the public methods of your class, including function1. When a client calls the public method there are certain expectations (inputs/outputs/processing performed) that can be tested, but the client shouldn't know or care if those expectations are met/enforced all in one method or through the use of multiple methods. So, from a clients perspective your code could have been written like this:

/*
 * @param $a An int to play with
 * @param $b An int to play with
 * @param $c An int to play with
 *
 * @throws InvalidArgumentException when either $a, $b, or $c are not an int
 *
 * @return A new int
 */
public function1($a, $b, $c) {
    if(!is_int($a)) throw new InvalidArgumentException('$a must be an int');
    if(!is_int($b)) throw new InvalidArgumentException('$b must be an int');
    if(!is_int($c)) throw new InvalidArgumentException('$c must be an int');

    $x = $a * $b;

    $y = $c + 1;

    return $x - $y;
}

If you had originally written your code like this and written tests for this behaviour, it would be possible for you to refactor your code, by adding function2 to share functionality with other methods safe in the knowledge that your public interface tests are making sure that the class still performs to clients as expected. This is where the term 'refactor with confidence' that you'll hear from time to time comes from.

If you start testing all of your private methods then you are going to end up tightly coupling your tests to the implementation (rather than behaviour) or your classes. This makes it much harder for your to refactor your code without breaking your tests and you can get to the point where tests are more of an overhead than a benefit.