[PATCH] Some tests for the system backend

Omair Majid omajid at redhat.com
Fri Mar 23 17:49:45 UTC 2012


Hi Roman,

On 03/22/2012 06:02 PM, Roman Kennke wrote:
>>> On 03/22/2012 11:14 AM, Jon VanAlten wrote:
>>>> Right.  In that case, I would say that this is indeed a smell... maybe
>>>> this means that an additional chained build() call would be needed,
>>>> passing in the results from one of the readers?  (or some default
>>>> values if that reader was no-good).
>>>
>>> I ran with the chaining idea. The result is attached. What do you think?
>>
>> I think it's almost fine.
>>

Thanks for looking over the patch.

>> - All those protected methods should be package private IMO. Protected
>> implies that they are meant to be overridden by subclasses, which is not
>> the case.
>> - I also feel that making parts of the code available for testing, while
>> leaving other parts (the actual file reading) untested is a slight code
>> smell. However, I don't have a quick better solution either. Maybe it
>> would help to provide another break-up and have build() hand over to a
>> package private build(String filename) to at least enable testing the
>> actual file reading and exception cases. (And if you then really would
>> want to test build(), mock the build(String filename) and check if
>> build() calls that other method with the correct (hardcoded) name...
>> however, then you would end up testing a partial mock, which can be a
>> code smell of itself... maybe not worth, unless it leads to a really
>> great redesign  ;-) ).
> 
> Another point:
> - In some cases we have really deep nesting, it would improve
> readability to split them up:
> 
>     protected HostCpuInfo getCpuInfo(Reader cpuInfoReader) throws
> IOException {
>         final String KEY_PROCESSOR_ID = "processor";
>         final String KEY_CPU_MODEL = "model name";
>         int cpuCount = 0;
>         String cpuModel = null;
>         if (cpuInfoReader != null) {
>             try (BufferedReader bufferedReader = new
> BufferedReader(cpuInfoReader)) {
>                 String line = null;
>                 while ((line = bufferedReader.readLine()) != null) {
>                     if (line.startsWith(KEY_PROCESSOR_ID)) {
>                         cpuCount++;
>                     } else if (line.startsWith(KEY_CPU_MODEL)) {
>                         cpuModel = line.substring(line.indexOf(":") +
> 1).trim();
>                     }
>                 }
>             }
>         }
> 
> 
> - You could avoid creating a BufferedReader there, and create it in the
> build() method directly (and have only one try block).
> - Do you really need that null-check there? (Add a test that checks that
> build() calls build(Reader) with a non-null value ;-) ).

I think the attached patch resolves most of these concerns. What do you
think?

> - Inside the while loop, you could extract a parseLine() method...
> however it modifies some state outside the loop scope, which might lead
> to...
> - Maybe extract all that parsing code into its own little class (this
> would separate the file/exception/etc handling from the actual parsing,
> which probably makes sense). Nice little episode by the Clean Code
> author: http://cleancoder.posterous.com/stub9-where-classes-go-to-hide
> 

I am not quite sure how I would do this. So I have not done it (yet). I
will keep it in the back of my mind though and try it some time.

Okay to commit?

Thanks,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: some-tests-for-system-backend-04.patch
Type: text/x-patch
Size: 30743 bytes
Desc: not available
URL: <http://icedtea.classpath.org/pipermail/thermostat/attachments/20120323/16fb8629/attachment-0001.bin>


More information about the Thermostat mailing list