farirat
commented
9 years ago @sotoz thank you for this first contrib !
I've reviewed the PR commit and here's my feedback before merging to master:
1. Some organizational stuff:
- Please consider referencing an issue number in each commit, this way we can know the context of each commit,
- You have to put a clear README in rst format (not md), just like Jasmin's one, where someone like me can follow some simple steps to use your api and test it,
- [optional] Maybe you need to think of packaging it, i think php has equivalent to pypi: packagist or composer, the project needs to be easily deployable by anyone without needing to go through the code and understand it's hierarchy foldering ... this will also be easy for new contribs to come and put their hands in.
- Testing script needs to be standardized, maybe considering phpunit, and the most important is to connect the project on travis-ci.org to run your tests on each push, just like Jasmin: https://travis-ci.org/jookies/jasmin , this has huuuge benefits for the project:
- Tests are automated, anyone can see how it's running without loosing time into deploying the app
- Each PR will have an automated test reported to help me, or any other one to accept/reject the PR based on the test result
- Using travis-ci, you can automate the installation of jasmin to use it's jcli console, that's just like installing any other requirement for testing, really ez !
- You may also add some coverage test to ensure your tests are covering at least, a major part of your app, Jasmin has a 87% coverage rate which is good, i considera minimum of 75 to 80% for a good coverage, the coverage is run on each test and will report back in travis-ci build log (you can see it there) and you can make the test fail if the coverage is lower than a defined threshold ...
- I see you considered documenting your code, that's great ! next step will be to rate the code, Jasmin is using this: https://www.codacy.com/app/support_3/jasmin/dashboard , you can schedule this for later as the codebase is still a 1 day old baby :)
2. Some api design stuff:
The class diagram looks pretty good, some points to pay attention for:
- Telnet connection and disconnection must be done in higher level than concrete classes like JasminGroupConnector, the way you did it will take 2 connections/disconnections to jCli for adding a group and a connector in the same page session, if the connection were in a higher level (maybe in JasminConnector, it is abstract anyhow ..) then we'll share the same connection for doing 2 separate actions.
- The .save() method must always check for returned status after sending the ok, this maybe implemented in JasminConnector and shared for all concrete inheriters as it's always the ok command to save, just a thought to clean up the design, same think for delete as well.
- I think all concrete classes must be named this way:
- JasminGroup
- JasminUser
- JasminSmppc
- JasminMtRoute
- JasminMoRoute
- These concrete classes must inherit one JasminObject class (just like Java's object) where we find .getAll(), .save() and .delete() factorized methods
- I see the telnet connection parameters is hardcoded in the constructor, i know we're okay this is a bad & dirty design, but it's okay for a proof of concept initial approach, let's think of a way to get it clean for future evolution.
- now let's go down to .getgroups(), seems to be a bad idea to get it there, since there's no way to filter the objects (groups, connectors or whatever), then let's change it to .getAll() to be clear enough, and place it in higher level (JasminObject) as explained earlier
Here's an example of a JasminUser and JasminGroup objects following the rules i've described earlier:
<?php
class JasminObject extends JasminConnector
{
public function getAll()
{
$result = $this->telnet->doCommand($this->command.' -l');
// Handle some exceptions, errors etc ...
return $resultArray // Associative array is a must
}
public function save()
{
$this->telnet->doCommand($this->command.' -a');
foreach $property as $this->properties {
$this->telnet->doCommand($property.' '.$this->{$property});
}
$this->telnet->doCommand('ok');
// Handle some exceptions, errors etc ...
// If everything went ok, return True
return true
}
public function delete()
{
$result = $this->telnet->doCommand($this->command.' -r '.$this->key);
// Handle some exceptions, errors etc ...
// If everything went ok, return True
return true // Associative array is a must
}
}
class JasminUser extends JasminObject
{
var $command = 'user'
var $key = ['uid']
var $properties = ['username', 'uid', 'password', 'gid', ... etc]
}
class JasminGroup extends JasminObject
{
var $command = 'group'
var $key = ['gid']
var $properties = ['gid']
}That's just a demo code, it's not working :)
I'll wait for your feedback or commits to enrich this PR and then merge it to master.
Fourat
sotoz
asgarciap
Added TelnetConnector for telnet communication and JasminGroupConnector for proof of concept. A test.php exists that executes the commands. The program executes the commands and echoes out the response from jcli.
Must see if this initial architecture fits our demands so we can pull that on master and continue from there.