Code review comment for lp:~gesha/linaro-license-protection/build-info-support

Revision history for this message
Georgy Redkozubov (gesha) wrote :

Stevan, thanks for review. Your notes are good.

>
>
> I would really like to see all the initialization inside the __construct
> class.
> Furthermore, empty strings need not be initialized. "private $fname;" is
> enough.
>
> There are very small amount of comments in the BuildInfo.php file.
> Fields like text_array are very non-descriptive, we should rename those as
> well.

Moved inititalization to constructor.

>
> We can avoid this whole mumbo by putting $fname as the attribute of the class
> and then only hold the relevant build info data for that file in text_array
> attribute. That's all we need for each HTTP request anyway.

That's a good point.

>
>
> All getFieldX methods should be united in one method.
> I.e. public function get($fieldName) - (if we remove $fname as suggested
> above)
> We'll get less code, less tests, and if we would need to implement new fields,
> we wouldn't need to add separate method and additional tests.
>

I refactored code and did the reduction.

>
> 801 === modified file 'tests/test_click_through_license.py'
> 802 --- tests/test_click_through_license.py 2012-05-17 18:44:59 +0000
> 803 +++ tests/test_click_through_license.py 2012-06-05 14:32:19 +0000
> 804 @@ -36,6 +36,9 @@
>
> Tests fail for these 3 new tests added..

That was by the mistype. Fixed.

« Back to merge proposal