On 05/01/2016 00:54, Mark Sheppard wrote:
Hi,
as per feedback below webrev has been updated
http://cr.openjdk.java.net/~msheppar/8134577/webrev.06/
change summary:
* List<NameService> nameServices replaced with Nameservice nameService
* references to nameServices removed
* private interface NameService added, with two implementation
PlatformNameService and HostsFileNameService
* Scanner created with UTF-8 charset
* removed StringTokenizer, replaced with String.split()
* comment handling extended, handling comments on line with mapping entry
* try with resources added to addMappingToHostsFile method in tests
I think this is starting to look good.
A few small comments:
- instantiateNameService (would createNameService be nicer?) still falls
back to PlatformNameService when the hosts file doesn't exist. Is that
expected?
- HostsFileNameService.hostsFile can be final.
- in removeComments then I assume you can just use indexOf and check for
-1 rather than contains + indexOf.
- it would be nice if NameService had some javadoc. Also the methods
don't need to be declared public.
- it would be nice if PlatformnameService and HostsFileNameService could
use /** */ style javadoc rather than //, just to keep the code consistent.
- ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577,
is there is separate issue for this?
- formatting. There are a few places where the indention is messed up (5
spaces instead of 4 in a few places). Also some of the throws XXX are
intended in many different ways. Blank lines can help readability but
seem to be inconsistently used here. There's clearly been a lot of
revisions on this issue and maybe the simplest is to just reformat this
section of the file in the IDE.
-Alan.
