[PATCH] Some tests for the system backend

Roman Kennke rkennke at redhat.com
Thu Mar 22 22:02:22 UTC 2012


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

You see I put a lot of maybes and probablys up there, don't let that
stop from committing as it is, I leave that fully up to you!

Cheers,
Roman





More information about the Thermostat mailing list