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