-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor methods for separation of concern #973
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good, thank you!
Please remove the intellij setup from the PR, so we can merge just the cleaned up code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Please do not commit configuration for your local IDE.
@@ -510,12 +510,34 @@ public boolean readExtraPeerDataFile(File extraPeerDataFile, int fileNumber) { | |||
return false; | |||
} | |||
Logger.normal(this, "extraPeerDataFile: "+extraPeerDataFile.getPath()); | |||
FileInputStream fis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this? Unused, that’s what it is!
@@ -510,12 +510,34 @@ public boolean readExtraPeerDataFile(File extraPeerDataFile, int fileNumber) { | |||
return false; | |||
} | |||
Logger.normal(this, "extraPeerDataFile: "+extraPeerDataFile.getPath()); | |||
FileInputStream fis; | |||
SimpleFieldSet fs = readFile(extraPeerDataFile); | |||
if (fs == null) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add braces for all blocks.
FileInputStream fis; | ||
SimpleFieldSet fs = readFile(extraPeerDataFile); | ||
if (fs == null) return false; | ||
if(fs == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent whitespace (cf. to the line directly above this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, wait, what? fs
has already been found to be null
so this if
can never be true
.
deleteExtraPeerDataFile(fileNumber); | ||
return true; | ||
} | ||
boolean parseResult = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this variable is not used outside the try
block, please declare it inside the try
block.
Logger.error(this, "Could not parse extra peer data: "+e2+ '\n' +fs.toString(),e2); | ||
gotError = true; | ||
} | ||
return !gotError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole construct seems to be over-complicated. Both parseResult
and gotError
(declared at the wrong end of the method) do not fulfill any real purpose here and can be removed without any damage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the changes for both of these files mixed into a single pull request? Please file separate pull requests; smaller pull requests are easier to handle than larger ones.
return false; | ||
} | ||
|
||
private boolean shutdownAnnouncement() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“shutdownAnnouncement” sounds like a command but the method doesn’t do anything, and it returns a value. What is actually meant here? Is the shutdown announcement being shown? Should it be? Should announcements be shut down? Please rename the method in a way that makes it clearer what it does.
@@ -359,6 +359,15 @@ boolean enoughPeers() { | |||
} | |||
return true; | |||
} | |||
if (shutdownAnnouncement()) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always use braces for blocks, and only one statement per line, please.
Sorry for barging into this, apparently I had started a review a couple of weeks ago but never got around to finish it. :) |
I reviewed just the change — the refactoring — not the original code. There are a lot more issues in the original code than just the ones fixed here, but I think they can be fixed in an independent PR. |
Some of the code is indeed the original code, just moved around, but some of it is new code, and for that code my comments still stand. 🙂 |
Changes proposed in this pull request:
Announcer.Java
This PR refactors the enoughPeers method by extracting the logic for shutting down the announcement into a separate method.
Changes:
shutdownAnnouncement
in the classenoughPeers
method to callshutdownAnnouncement
where appropriateRationale:
Note:
DarknetPeerNode.Java
This PR refactors the readExtraPeerDataFile method by extracting the file reading logic into a separate method.
Changes:
readFile
that takes a file path as input and returns the file contents as a stringRationale:
Note: