[PATCH] Some tests for the system backend

Roman Kennke rkennke at redhat.com
Mon Mar 26 19:19:16 UTC 2012


Hi Omair,

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

Yep, looks good now! I like the ProcDataSource idea!

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

That is fine.

I found more things to look at in the future (but which are really not
related to your change): The MemoryStatBuilder has a while loop with a
long if-else chain. This could be nicely refactored by keeping all those
interim long values in a table, and use the key to address that table,
then build the final object out of the values in that table. But this is
only a rather minor nice-ification for doing later.

> Okay to commit?

Sure, please go ahead.

Cheers,
Roman





More information about the Thermostat mailing list