BMI

bmi.php

<?php
/*
 * calc_bmi
 * The structure of this function is misleading and overcomplicated.
 * The function returns false or a value, so any program calling it must handle
 * either false or a value.
 * Further, the function is burdened with mistrust and has misleading default arguments.
 * The default arguments suggest that the function expects integers, but it actually handles strings and many other possible values.
 */

  function calc_bmi($height = 1, $weight = 1) {
    // Code is written once, usually, and read many times. Modification should be straightforward and simple.
    // Therefore, one-liners are discouraged unless they are exceedingly clear or idiomatic.
    // Combining a boolean or with a ternary and exploiting the short-circuiting nature to effect a guard function
    // is unnecessarily complex.

    // If at all possible, sanitize input before giving it to your function. This reduces complexity, simplifies the function,
    // and adheres to the [single responsibility principle](https://en.wikipedia.org/wiki/Single_responsibility_principle). 

    return !calc_to_int($height) || !calc_to_int($weight) ? false :
    703 * $weight / ($height * $height);

    // if you have sanitized or trust the input, then this function is just:
    return 703 * $weight / ($height * $height);
  }

  // The calc_to_int function guard and then returns an integer. The formula to calcualte BMI will work with floating point numbers
  // So I would suggest using [floatval](http://php.net/manual/en/function.floatval.php) instead.
  // The expression is a good way to scrub non-digit characters from an input string, but probably doesn't
  // need to be this deep in the logic. You should sanitize input as close to the input as possible, so if this is supposed to be
  // a number, then sanitize and parse it there, then assign it and pass it in.
  function calc_to_int($string) {
    // This is a guard clause. It is bizarre to use a replace function's return value as a truthy
    // predicate for an if clause.
    // You should store the expression in a variable and use (preg_match())[http://php.net/manual/en/language.types.boolean.php]
    // `if (preg_match($pattern, $string)) { ... }`

    if (!preg_replace('/[^\d\.]/', '', $string)) {
      return false;
    }
    else {
      // Single-responsibility principle again. Simply because it complicates here where there is no need.
      // I wouldn't always advise extracting nested functions, but as I repeatedly point out, you should already
      // have clean input before using intval or floatval. A string manipulation function paired with a parsing function seems like
      // two different domains that should probably have been resolved a long time ago.
      return intval(preg_replace('/[^\d\.]/', '', $string));
    }
  ?>