Skip to main content
Fix typos
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31
  • The mb_strlen($configFileNameWithPath) > 0 checks don't need the > 0 part at the end. mb-strlenmb_strlen() will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value in mb_strlen() -- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just use strlen().

  • IMPORTANT: A constructor returns nothing. You should not be returning true. Please read Constructor returning value?.

  • $envVarNames and $settingConfigurationName are related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ a foreach() loop instead of counting the length of an array on every iteration.

    private $envConfigNames = [
        'EHJWT_JWT_SECRET' => 'jwtSecret',
        'EHJWT_DSN' => 'dsn',
        'EHJWT_DB_USER' => 'dbUser',
        'EHJWT_DB_PASS' => 'dbPassword',
    ];
    
    private function setConfigurationsFromEnvVars(): void
    {
        foreach ($this->$envConfigNames as $envName => $configName) {
            $value = getenv($envName);
            if (strlen($value)) {
                $this->configurations[$configName] = $value;
            }
        }
    

    Use this lookup array again in setConfigurationsFromConfigFile() and setConfigurationsFromArguments().

  • Instead of gettype($configFileSettings) !== 'array', it is more concise to use !is_array($configFileSettings).

  • I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as `$value = $$settingName;$value = $$settingName;

  • You never need to check if a variable/element isset() before unset()ing it. Remove the condition and just unset() it.

  • I do not endorse the unconditional return of true in methods. It is not informative, it is just useless noise. If there is no chance of returning false, just don't return anything (:void).

  • I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name). createToken() seems like the values should be directly fed into the implode() call.

  • I recommend declaring the return types on all methods (except the constructor). This will help with debugging and enforce strict/predictable types in your application. public function loadToken(string $tokenString): bool

  • Consistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert where to WHERE)

  • Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.

  • I do not like that you are creating a new pdo connection for every query. You should modify makeRevocationTableDatabaseConnection() so that if there is already an established connection, then it is re-used.

  • Return the boolean evaluation for more concise code.

    private function verifyThreeMembers(array $array): bool
    {
        return count($array) === 3;
    }
    

    For methods that return a boolean value, I often start the method name with is or has (or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.

  • With pdo, I prefer to pass the values directly into the execute() function to avoid the bindParam() calls.

  • Use fetchAll() when returning an entire unaltered result set instead of iteratively fetching the result set.

    return $stmt->fetchAll(PDO::FETCH_ASSOC);
    
  • The mb_strlen($configFileNameWithPath) > 0 checks don't need the > 0 part at the end. mb-strlen() will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value in mb_strlen() -- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just use strlen().

  • IMPORTANT: A constructor returns nothing. You should not be returning true. Please read Constructor returning value?.

  • $envVarNames and $settingConfigurationName are related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ a foreach() loop instead of counting the length of an array on every iteration.

    private $envConfigNames = [
        'EHJWT_JWT_SECRET' => 'jwtSecret',
        'EHJWT_DSN' => 'dsn',
        'EHJWT_DB_USER' => 'dbUser',
        'EHJWT_DB_PASS' => 'dbPassword',
    ];
    
    private function setConfigurationsFromEnvVars(): void
    {
        foreach ($this->$envConfigNames as $envName => $configName) {
            $value = getenv($envName);
            if (strlen($value)) {
                $this->configurations[$configName] = $value;
            }
        }
    

    Use this lookup array again in setConfigurationsFromConfigFile() and setConfigurationsFromArguments().

  • Instead of gettype($configFileSettings) !== 'array', it is more concise to use !is_array($configFileSettings).

  • I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as `$value = $$settingName;

  • You never need to check if a variable/element isset() before unset()ing it. Remove the condition and just unset() it.

  • I do not endorse the unconditional return of true in methods. It is not informative, it is just useless noise. If there is no chance of returning false, just don't return anything (:void).

  • I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name). createToken() seems like the values should be directly fed into the implode() call.

  • I recommend declaring the return types on all methods (except the constructor). This will with debugging and enforce strict/predictable types in your application. public function loadToken(string $tokenString): bool

  • Consistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert where to WHERE)

  • Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.

  • I do not like that you are creating a new pdo connection for every query. You should modify makeRevocationTableDatabaseConnection() so that if there is already an established connection, then it is re-used.

  • Return the boolean evaluation for more concise code.

    private function verifyThreeMembers(array $array): bool
    {
        return count($array) === 3;
    }
    

    For methods that return a boolean value, I often start the method name with is or has (or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.

  • With pdo, I prefer to pass the values directly into the execute() function to avoid the bindParam() calls.

  • Use fetchAll() when returning an entire unaltered result set instead of iteratively fetching the result set.

    return $stmt->fetchAll(PDO::FETCH_ASSOC);
    
  • The mb_strlen($configFileNameWithPath) > 0 checks don't need the > 0 part at the end. mb_strlen() will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value in mb_strlen() -- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just use strlen().

  • IMPORTANT: A constructor returns nothing. You should not be returning true. Please read Constructor returning value?.

  • $envVarNames and $settingConfigurationName are related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ a foreach() loop instead of counting the length of an array on every iteration.

    private $envConfigNames = [
        'EHJWT_JWT_SECRET' => 'jwtSecret',
        'EHJWT_DSN' => 'dsn',
        'EHJWT_DB_USER' => 'dbUser',
        'EHJWT_DB_PASS' => 'dbPassword',
    ];
    
    private function setConfigurationsFromEnvVars(): void
    {
        foreach ($this->$envConfigNames as $envName => $configName) {
            $value = getenv($envName);
            if (strlen($value)) {
                $this->configurations[$configName] = $value;
            }
        }
    

    Use this lookup array again in setConfigurationsFromConfigFile() and setConfigurationsFromArguments().

  • Instead of gettype($configFileSettings) !== 'array', it is more concise to use !is_array($configFileSettings).

  • I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as $value = $$settingName;

  • You never need to check if a variable/element isset() before unset()ing it. Remove the condition and just unset() it.

  • I do not endorse the unconditional return of true in methods. It is not informative, it is just useless noise. If there is no chance of returning false, just don't return anything (:void).

  • I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name). createToken() seems like the values should be directly fed into the implode() call.

  • I recommend declaring the return types on all methods (except the constructor). This will help with debugging and enforce strict/predictable types in your application. public function loadToken(string $tokenString): bool

  • Consistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert where to WHERE)

  • Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.

  • I do not like that you are creating a new pdo connection for every query. You should modify makeRevocationTableDatabaseConnection() so that if there is already an established connection, then it is re-used.

  • Return the boolean evaluation for more concise code.

    private function verifyThreeMembers(array $array): bool
    {
        return count($array) === 3;
    }
    

    For methods that return a boolean value, I often start the method name with is or has (or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.

  • With pdo, I prefer to pass the values directly into the execute() function to avoid the bindParam() calls.

  • Use fetchAll() when returning an entire unaltered result set instead of iteratively fetching the result set.

    return $stmt->fetchAll(PDO::FETCH_ASSOC);
    
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31

  • The mb_strlen($configFileNameWithPath) > 0 checks don't need the > 0 part at the end. mb-strlen() will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value in mb_strlen() -- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just use strlen().

  • IMPORTANT: A constructor returns nothing. You should not be returning true. Please read Constructor returning value?.

  • $envVarNames and $settingConfigurationName are related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ a foreach() loop instead of counting the length of an array on every iteration.

    private $envConfigNames = [
        'EHJWT_JWT_SECRET' => 'jwtSecret',
        'EHJWT_DSN' => 'dsn',
        'EHJWT_DB_USER' => 'dbUser',
        'EHJWT_DB_PASS' => 'dbPassword',
    ];
    
    private function setConfigurationsFromEnvVars(): void
    {
        foreach ($this->$envConfigNames as $envName => $configName) {
            $value = getenv($envName);
            if (strlen($value)) {
                $this->configurations[$configName] = $value;
            }
        }
    

    Use this lookup array again in setConfigurationsFromConfigFile() and setConfigurationsFromArguments().

  • Instead of gettype($configFileSettings) !== 'array', it is more concise to use !is_array($configFileSettings).

  • I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as `$value = $$settingName;

  • You never need to check if a variable/element isset() before unset()ing it. Remove the condition and just unset() it.

  • I do not endorse the unconditional return of true in methods. It is not informative, it is just useless noise. If there is no chance of returning false, just don't return anything (:void).

  • I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name). createToken() seems like the values should be directly fed into the implode() call.

  • I recommend declaring the return types on all methods (except the constructor). This will with debugging and enforce strict/predictable types in your application. public function loadToken(string $tokenString): bool

  • Consistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert where to WHERE)

  • Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.

  • I do not like that you are creating a new pdo connection for every query. You should modify makeRevocationTableDatabaseConnection() so that if there is already an established connection, then it is re-used.

  • Return the boolean evaluation for more concise code.

    private function verifyThreeMembers(array $array): bool
    {
        return count($array) === 3;
    }
    

    For methods that return a boolean value, I often start the method name with is or has (or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.

  • With pdo, I prefer to pass the values directly into the execute() function to avoid the bindParam() calls.

  • Use fetchAll() when returning an entire unaltered result set instead of iteratively fetching the result set.

    return $stmt->fetchAll(PDO::FETCH_ASSOC);