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

Revision history for this message
Stevan Radaković (stevanr) wrote :

Great work gesha. Approved.

> 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.

review: Approve

« Back to merge proposal