HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Geocoding hook for a TYPO3 extension

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
geocodingextensionforhooktypo3

Problem

I'm currently developing a TYPO3 Extension in PHP and developed a Hook.
TYPO3 has the concept of Hooks where you can execute your own code as specific points. As defined in our definition of done, we want to PHPUnit-test our code base.

I did some small tests before, so I'm familiar with the basic concepts. Below you can find the class to test, and all tests for the class. With the contained annotations, it will cover 100%. But as I'm pretty new to this topic, it would be helpful to get feedback whether the code, and specially tests are okay. Do they cover what I need? Can the code be improved, especially the tests? They are messed by a huge setup.

Class to test Classes/Hook/DataMapHook.php:

```

*/
class DataMapHook
{
/**
* Fieldnames that trigger geo decode.
*
* @var array
*/
protected $fieldsTriggerUpdate = ['address', 'city', 'country', 'zip'];

/**
* Table to work on. Only this table will be processed.
*
* @var string
*/
protected $tableToProcess = 'fe_users';

/**
* Hook to add latitude and longitude to locations.
*
* @param string $action The action to perform, e.g. 'update'.
* @param string $table The table affected by action, e.g. 'fe_users'.
* @param int $uid The uid of the record affected by action.
* @param array $modifiedFields The modified fields of the record.
*
* @return void
*/
public function processDatamap_postProcessFieldArray( // @codingStandardsIgnoreLine
$action, $table, $uid, array &$modifiedFields
) {
if(!$this->processGeocoding($table, $action, $modifiedFields)) {
return;
}

$geoInformation = $this->getGeoinformation(
$this->getAddress($modifiedFields, $uid)
);
$modifiedFields['lat'] = $geoInformation['geometry']['location']['lat'];
$modifiedFields['lng'] = $geoInformation['geometry']['location']['lng'];
}

/**
* Check whether to fetch

Solution

You shouldn't be needing to have formatter comments like these:

// @codingStandardsIgnoreLine


Don't ignore coding standards, follow them.

You shouldn't be calling static methods like this:

\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
        $record,
        $modifiedFields
    );


That's just messy. Consider importing the parent class instead.

Instead of this messy string concatenation, consider using an array join instead:

return $record['address'] . ' ' . $record['zip'] . ' ' . $record['city'] . ' ' . $record['country'];


Into

return [implode(",", array($record['address'], $record['zip'], $record['city'], $record['country']));


Adding to the above point, why do you have no problem concatenating all those strings on one line, but you have a problem with two parameters on one line?

Stick to a standard throughout please.

\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
    $record,
    $modifiedFields
);


Magic numbers

This one gets a header because I have a few points on it:

You shouldn't have scenarios like these:

if ($response['status'] === 'OK') {
        return $response['results'][0];
    }


What is at the first index of $response['results']? Why do you need to return it?

Try to make your code intentions a little more clear.

throw new \Exception(
        'Could not geocode address: "' . $address . '". Return status was: "' . $response['status'] . '".',
        1450279414
    );


What is 1450279414?

Try to make your code's intentions and reasoning a little bit more clear:

$cupsOfCoffeeIveDrank = 1450279414;
throw new \Exception(
    'Could not geocode address: "' . $address . '". Return status was: "' . $response['status'] . '".',
    $cupsOfCoffeeIveDrank
);


There, that's better.

Functions and variables in PHP are named in snake_case, not camelCase:

processGeocoding


process_geocoding


This is common across all functions in:

$expectedResult = ['title' => 'test'];
    $modifiedFields = $expectedResult;


Consider moving it to a more accessible place so it doesn't need to be replicated.

You shouldn't be having actual data hardcoded in, pass it in from a database or config file for better maintainability.

->will(self::returnValue([
            'address' => 'An der Eickesmühle 38',
            'zip' => '41238',
            'city' => 'Mönchengladbach',
            'country' => 'Germany',

Code Snippets

// @codingStandardsIgnoreLine
\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
        $record,
        $modifiedFields
    );
return $record['address'] . ' ' . $record['zip'] . ' ' . $record['city'] . ' ' . $record['country'];
return [implode(",", array($record['address'], $record['zip'], $record['city'], $record['country']));
\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
    $record,
    $modifiedFields
);

Context

StackExchange Code Review Q#114871, answer score: 2

Revisions (0)

No revisions yet.