I've been reviewing the darknet node reference exchange Android application and associated patches. Nitesh, would you like to participate in this review? I've attached feedback on the Fred and MDNSDiscovery patches.
I haven't done code review in this kind of situation, so if there's a better way to do it I'd be happy to hear it. Some issues are just cosmetic, and others are design or readability problems. Sometimes it's not clear whether something is actually correct. Thanks, Steve
Overall comments: - I find it difficult to follow a single return value variable that's modified throughout the function and returned in one place at the end. Returning a literal from multiple places might be preferable. - Many places use repetitive and exception catching and logging. Reducing that would be worth looking into - I think Java 7 allows catching multiple types of exceptions in one statement. It might not be usable in that case though if details about the exception cannot then be logged. - There seems to be a fair amount of duplication of SSL-related classes between the projects. (BCSSLNetworkInterface/BCModifiedSSL) - It'd be preferable to avoid trailing whitespace and mixing tabs and spaces. Files should end with a newline by convention. - Commented-out code should be removed. If it's still needed the source repository will have it. - Please don't print to the console. This is primarily suited to debugging. - For readability purposes higher-level methods should be higher in the file than the lower-level methods that they call. It follows that entry points should be higher in the file. - Constants should be static final. - More descriptive commit messages would be helpful. - Sun classes are an internal API, so the Base64 should come from somewhere else, such as Apache Commons Codec. See http://www.oracle.com/technetwork/java/faq-sun-packages-142232.html It may be worth noting that elsewhere in Fred uses Base64 that omits padding characters. Fred: git diff -w 87b6737ad344e1c1a0b6a831ada576e5fc37c853^..9ada38986219a0efcead67f220a8c98ba3309758 - The temporary references file is repeatedly read from disk. Keeping the file on disk up-to-date seems necessary, but perhaps it could be kept in memory after an initial load to avoid always having to read it again. If not then the property-reading code should be a single function instead of repeated. - Can there be multiple simultanious connections to the app? Are the home node and the app paired? - src/freenet/io/BCSSLNetworkInterface.java - It would make sense to me to replace the existing SSL implementation with this BC-based one instead of making a parallel one. - src/freenet/darknetapp/ECDSA.java - This seems to be similar to src/freenet/crypt/ECDSA.java in that it reads it stuff from a properties file and signs and verifies. Does this implementation need to exist? - getPublicKey() and getSignature(): no need to check if(!generated) and then if(generated) when initialize() does generation. - getPublicKey(): The key variable seems unnessary - could return the value directly. - Good job on the ordering - this seems to go high to low level. - generateProperties(): maybe a more descriptive name would be "generateKeypair()"? - pullProperties(): maybe a more descriptive name would be "loadKeypair()"? - initialize(): This loads a newly created file. Is that useful? - src/freenet/l10n/freenet.l10n.en.properties - "DarknetConnectionsToadlet.addNew" is a duplicate localization of "Submit". Would it make sense to use the browser's default translation of "Submit" for a form? If not I'd expect Fred localization to already have it. - It doesn't make sense to me to add a setting that asks not to be changed. If that's the case it's not really a setting then. Perhaps a button to clear the temporary file would be more appropriate. - src/freenet/darknetapp/DarknetAppServer.java - This getLogger stuff is verbose - is it practical to just fetch it once and keep it around? Is it reasonable that this uses the java.util Logger instead of the Fred one? - int newDarknetPeersCount - "new" is vague. Would something like numPendingPeers be more descriptive? - myThread is vague. What does it do? Does it dispatch connections? - start() does nothing if enabled is false? I guess that makes sense to be at this layer. - It's not clear to me from the name what maybeGetNetworkInterface() does. Why the "maybe?" What would it do instead? - Something like this comment about 0.0.0.0 bindTo should be in the localization description of the option as well. - configureFile() sounds like it would configure a file. If it was reading a configuration file readConfig() or loadConfig() would be good. It's not configuration, though - it's state persistance. - "Exchange" is spelled incorrectly here and elsewhere. - Passing the logger the exception will likely get a nice stacktrace or something as well. If just string appending there's no whitespace and every word is capitalized. - I don't understand why there's a static unsynchronized changeNewDarknetPeersCount() and a synchronized non-static one? Having a single synchronized non-static one might make more sense? - Why does changeNewDarknetPeers() always log "unsynchronized peer count"? It seems like debugging output if it always happens. - "//Modified from SimpleToadletServer to use BCSSLNetworkInterface instead of SSLNetworkInterface" suggests that if nothing else the BCSSLNetworkInterface replace the SSLNetworkInterface. - "In case of an attack where our homeNode is bombarded with new references," - So there's no key exchange between the home node and the app? If an attacker is on the home network that's already problematic. I don't know that this seems like a compelling use case to me. - "// Copied from SimpleToadletServer" - Can this code duplication be avoided somehow? - "Although, the MDNS plugin might have to be restarted as it would still be broadcasting the old port" - How about triggering the MDNS plugin restart? - run() wait()s while synchronized? Does that block other threads? Will it ever lead to a deadlock? - Why is there a local-scope finishedStartup and an instance one? What's the difference? - "// Handle request here" does not give more information. Context and naming gives that much. - darknetAppConnections increment and decrement should both be in the run(): doing the increment in another thread means it might decrement before it increments. - It's not clear to me what "NativeThread.HIGH_PRIORITY-1;" is. Is subtracting higher priority? Why is it this priority? Is there a named constant for the specific level? - finishStart(): "//TODO: Change this " - change what? - src/freenet/darknetapp/DarknetAppConnectionHandler.java - Why are the streams instance state? Do they persist beyond a single method? - The return value is not "done" - false closes the socket, which would be useful to document. - Why were these readLine() constants chosen? Should they be named constants somewhere? - int nFriendsRef could perhaps have a more descriptive name. "numReferences" perhaps? - processConnection() - No need for a command variable outside of debugging purposes. Also the comment doesn't seem helpful. - Under what circumstances is the socket kept open and why? - finish() - Setting instance references to null... if this object's lifetime is that of one request why bother? - Also why does it have instance state anyway? Passing the input stream as an argument would make sense to me. - processReferences() - maybe "receieveReferences()"? - Passing the stream as an argument would make it clearer that the stream is being used here. - Adding a message to the IOException would be helpful. - src/freenet/clients/http/DarknetAddRefToadlet.java - Good job keeping with the existing style of this code (contents in ConnectionToadlet for some odd reason) even though it's weird. - src/freenet/clients/http/ConnectionsToadlet.java - What was the reasoning behind using Properties instead of SFS? I don't have any particular problem with it, but I'm curious as it's inconsistent with the rest of the Fred codebase. - Why is tempNewDarknetRefs instance state instead of an argument? - The "Put the code in a loop" comment doesn't seem helpful. - No error is logged or written to the page if there's an IOException while reading the properties file. - trustS and visibilityS seem like strange variable names. Why? - is isOpennet() applicable to the error page here if it's always darknet? - Agreed on the layer violation. Moving the null check into darknetpeer and throwing an illegal argument exception seem appropriate. - This noderef newline preperation stuff could really use some comments. What's the regex for? Do the newlines really need that much treatment? This would be more appropriate as part of the noderef parser. - It was very unclear that the results hashmap is used to count the number of peers per result code. A comment would be helpful. - changeNewDarknetPeersCount() static version used here. Is there not an instance accessible? Could this lead to running the method with a node other than an instance's one? - void drawNewDarknetPeersAuthBox() - On IOException the page continues rendering. Would it make more sense to log an error and skip rendering the box? Maybe it should modify the page after making sure the file is okay. Similarly on file not found the page should probably reflect it somehow. - Can the radio buttons be labelled instead of having text next to them? - src/freenet/node/Node.java - start != initialization? What's the distinction, and why does initialization need to be marked? MDNSDiscovery: git diff -w 7c451a3297cc62f99398993a21ee674645dca8ab^..f37bc110f402264671871f0054e9ed69a0d2bcf0 - Some class header comments were still IDE the template. They should be filled out or removed. - build.xml - Changing the stock build file to fit one's setup is usually not appropriate. In this case it might be reasonable to remove the mention of CVS. Surprisingly this build.xml does not support an override.properties; it should. - Why make the dist target no longer depend on the tests? - After changing the existing paths to fit my system, I also had to add Bouncy Castle to the path for this to build. - src/freenet/crypt/BCModifiedSSL.java, src/freenet/darknetapp/ECDSA.java - Plugins can use classes in Fred, and Fred already has these. They should not be duplicated in the plugin. - src/plugins/MDNSDiscovery/MDNSDiscovery.java - The linked lists ourAdvertisedServices and ourDisabledServices should have specified types. - data2sign is clear, but ... how about "data" or "dataToSign" for consistency with other names? - Intermediate named variables for filling this payload buffer (like index ranges) would be good for readability. Comments too. - It's an interesting technique to have both the divided size and the modulo. I assume the reasoning for that is space? What's this technique called? (Is space a concern?) - Single-line if/else clauses without braces are not recommended by the project coding style for maintainance reasons. - It'd be useful to move things like nodeConfig.get("fproxy").getBoolean("enabled") && !nodeConfig.get("fproxy").getOption("bindTo").isDefault() into named booleans so that what they mean is clear.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devl mailing list [email protected] https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
