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

Hi gesha,

Your additions look great!

Few small pointers tho:

92 === added file 'licenses/BuildInfo.php'
93 --- licenses/BuildInfo.php 1970-01-01 00:00:00 +0000
94 +++ licenses/BuildInfo.php 2012-06-05 14:32:19 +0000
95 @@ -0,0 +1,207 @@
96 +<?php
97 +
98 +class BuildInfo
99 +{
100 + private $text_array = array();
101 + private $fields_defined = array("Format-Version", "Files-Pattern",
102 + "Build-Name", "Theme", "License-Type", "OpenID-Launchpad-Teams",
103 + "Collect-User-Data", "License-Text");
104 + private $multiline_vars = array("License-Text");
105 + private $search_path = '';
106 + private $fname = '';

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.

132 + private function getInfoForFile($fname)
133 + {
134 + /**
135 + * Get array of fields for corresponding file
136 + */
137 + foreach (array_keys($this->text_array) as $key)
138 + if ($key != 'Format-Version') {
139 + $files = glob($this->search_path."/".$key);
140 + foreach ($files as $file)
141 + if ($file == $this->search_path."/".$fname)
142 + return $this->text_array[$key];
143 + }
144 + return array();
145 + }

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.

165 + public function getTheme($fname)
166 + {
167 + $info = $this->getInfoForFile($fname);
168 + if (array_key_exists('Theme', $info))
169 + return $info["Theme"];
170 + else
171 + return false;
172 + }

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.

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