Add factory static method

[tomothumb]

Added static method for instance in the case of IP address with subnet mask.

$sub = IPv4\SubnetCalculator::factory('192.168.112.203/23');

[coveralls]

Coverage remained the same at 100.0% when pulling 475327c on tomothumb:develop into 9408f65 on markrogoyski:develop.

[markrogoyski]

Hi. Thanks for your interesting in IPv4 Subnet Calculator.

May I ask, what problem are you trying to solve with this pull request? The constructor is simple, and there are not any polymorphic subclasses that might get created based on some criteria. Is it just the case where you had a string that contained both the IP address and the network size and wanted a way to create a SubnetCalculator without having the using code to have to split the pieces apart to use the existing constructor?

Thanks,
Mark

[tomothumb]

Is it just the case where you had a string that contained both the IP address and the network size and wanted a way to create a SubnetCalculator without having the using code to have to split the pieces apart to use the existing constructor?

Yes, it is.
There is no problem on this lib.

[markrogoyski]

Hi @tomothumb,
Thanks for your patience with this pull request.

The question I have been thinking about is does the convenience of this new factory method outweigh the complexity introduced to the interface of constructing a SubnetCalculator? Having multiple options is convenient, but also introduces complexity as the user now has to understand two ways to construct the object and make a choice.

The /24 etc. notation seems to be pretty common, however. Just doing some searches, I can see that the English Wikipedia article for Subnetwork and some Cisco articles on networks all use the / notation.

So, I am OK with adding this factory method.

I think we need more unit tests on this new factory method. There should be extensive test cases with networks of all sizes from /1 to /32 and compare the creation methods to determine that using the constructor and the factory method produce the same object.

[tomothumb]

@markrogoyski
Thank you for considering my request.
I think /24 style is common too.

I also agree about the need for more testing.
I think there are several implementations on stable library and their unit tests in other languages.
I will quickly check them this weekend or next.

[markrogoyski]

Hi @tomothumb,

The current library has 100% unit test coverage and thoroughly tests using all kinds of network sizes. There are sufficient tests.

What we need for this pull request, is to validate that using the static factory method produces the expected SubnetCalculator. If it does that, then we know that the existing unit tests also apply.

For example, if you have a test like:

public function testStaticFactoryMethod()
{
    // Given
    $ipAddress = '192.168.3.5;
    $networkSize = 24;
    $expectedSubnetCalculator = new SubnetCalculator($ipAddress, $networkSize);

    // When
    $subnetString = "$ipAddress/$networkSize";
    $subnetCalculator = SubnetCalculator::create($subnetString);

    // Then
    $this->assertSame($expectedSubnetCalculator->getIPAddress(), $subnetCalculator->getIPAddress());
    $this->assertSame($expectedSubnetCalculator->getNetworkSize(), $subnetCalculator->getNetworkSize();
    // ... additional assertions for reporting
}

Basically, we just need unit tests that show that the new factory creation method creates the same SubnetCalculator if we had broken up the pieces and called the constructor ourselves.

I think a single unit test with a data provider for various IP addresses amongst all 32 network sizes and that will be sufficient to validate everything works as expected.

[tomothumb]

ah-ha, I got it.
I hadn't checked whole tests.

Finally, Is required test the one which you wrote now, then?

[markrogoyski]

Yes. Basically add a test like the one I wrote. Use a data provider so you have 32 network sizes all being tested. There are many existing tests that use a data provider to provide data to a test for 32 different network sizes.

I can help write the tests if you have any questions. Thanks for following up.