Skip to content

Conversation

machadoug
Copy link
Contributor

Added simple sanity check because doesn't make sense to have clusters with less than 2 locations.
It also has to be an integer and positive number.

Added simple sanity check because doesn't make sense to have clusters with less than 2 locations.
It also has to be an integer and positive number.
{
$this->minLocations = $limit;
// simple sanity check. It doesn't make sense to have clusters with less than 2 locations
$this->minLocations = (int)($limit > 2 ? $limit : 0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of automatically changing, can you make it throw an exception instead? (and add the @throws to this PHPDoc)

if ($limit < 2) {
    throw new Exception("It doesn't make sense to have clusters with less than 2 locations.");
}

$this->minLocations = $limit;

And can you also fix up ClustererTest.php, which relies on clusters of 1 being possible?

testClusterCoordinatesException calls setMinClusterLocations with 1, which can safely be changed to 2.
testClustered also calls it with 1. You can also change it to 2, but part of the test will have to be changed then:

// all coordinates should be clustered
// the 3 Brussels railway stations should form 1 cluster
$clusters = $clusterer->getClusters();
$this->assertEquals(count($coordinates) - 2, count($clusters));
$this->assertEquals(3, $clusters[3]->total);
$this->assertEquals(0, count($clusterer->getCoordinates()));
// cluster must also have empty coordinate array
$this->assertCount(0, $clusters[0]->coordinates);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I working on a script to automatically set the number of minLocations based on the zoom level and on the number of markers on the map. Sometimes this script provides a small number between 0 and 2. This sanity check is being done in this script, however I believe it should be done in the library in order to avoid problems. Throwing an exception in this case (and in my opinion) is not advised, due to that it is more appropriate to just change it. Also adding an exception would cause an incompatibility with older versions, requiring developers to adjust their code.
I would only throw an exception if debug mode was on....

@matthiasmullie matthiasmullie force-pushed the master branch 3 times, most recently from 5298af7 to bcaac43 Compare November 18, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants