search

zexa / interview-task

A comission calculator
0 stars 0 forks source link

Template php is misnamed #8

Open Dragas opened 5 years ago

Dragas commented 5 years ago

All parsers seem to extend this particular class, but from semantic point of view it is not clear why or what this class does in particular. Its functionality may be moved (Template#setFile) should be moved to constructor instead if you intend to use parsers per file. Otherwise you should pass the file path as argument into parse method.

zexa commented 5 years ago

Hello @Dragas ,

Thank you for your feedback.

As discussed in #3, these parsers are designed to be reusable, i.e. multiple files can be used on the same parser. Also, setFile provides useful functionality for debugging purposes.

Though I agree that 'Template' might be too ambiguous. Could you perhaps provide ideas for a more descriptive class name for the current 'Template'?

Dragas commented 5 years ago

They do not seem to expose a "reusable API". In addition, CSV parser seems to add behavior of "executing a function per parsed line". How would the equivalent work for JSONParser?

While not related to php, setFile method might be prone to race conditions, hence the suggestion to pass it to method parse as argument instead.

The name template on its own does not make much sense. Take the following example:

/**
 *  @var Template
 */ 
$parser = new JSONParser()

The IDE will index $parser field as Template instace. In the long run, when you're passing along the abstract definition, the meaning gets lost. As a result code readability is reduced. As a result, name Parser would be more semantic.