Skip to content

Refactor methods for separation of concern #973

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.

Sign up for GitHub

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

Open
anonstudy wants to merge 1 commit into
base: next
Choose a base branch
from
Open

Refactor methods for separation of concern #973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .idea/.gitignore
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/compiler.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions .idea/gradle.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions .idea/jarRepositories.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions .idea/misc.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/vcs.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 11 additions & 6 deletions src/freenet/node/Announcer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,15 @@ boolean enoughPeers() {
}
return true;
}
if (shutdownAnnouncement()) return true;
Copy link
Copy Markdown
Contributor

@Bombe Bombe Sep 15, 2024

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.


synchronized(timeGotEnoughPeersLock) {
timeGotEnoughPeers = -1;
}
return false;
}

private boolean shutdownAnnouncement() {
Copy link
Copy Markdown
Contributor

@Bombe Bombe Sep 15, 2024

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.

boolean killAnnouncement = false;
if((!node.getNodeUpdater().isEnabled()) ||
(node.getNodeUpdater().canUpdateNow() && !node.getNodeUpdater().isArmed())) {
Expand All @@ -382,7 +391,7 @@ boolean enoughPeers() {
}

}

if(killAnnouncement) {
node.getExecutor().execute(new Runnable() {

Expand All @@ -395,7 +404,7 @@ public void run() {
node.getPeers().disconnectAndRemove(pn, true, true, true);
}
}

});
return true;
} else {
Expand All @@ -411,10 +420,6 @@ public void run() {
return true;
}
}

synchronized(timeGotEnoughPeersLock) {
timeGotEnoughPeers = -1;
}
return false;
}

Expand Down
45 changes: 26 additions & 19 deletions src/freenet/node/DarknetPeerNode.java
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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.

Original file line number Diff line number Diff line change
Expand Up @@ -510,12 +510,34 @@ public boolean readExtraPeerDataFile(File extraPeerDataFile, int fileNumber) {
return false;
}
Logger.normal(this, "extraPeerDataFile: "+extraPeerDataFile.getPath());
FileInputStream fis;
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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!

SimpleFieldSet fs = readFile(extraPeerDataFile);
if (fs == null) return false;
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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.

if(fs == null) {
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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

Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024 •
edited
Loading

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 non-null so this if can never be true.

Logger.normal(this, "Deleting corrupt (too short?) file: "+extraPeerDataFile);
deleteExtraPeerDataFile(fileNumber);
return true;
}
boolean parseResult = false;
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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.

try {
parseResult = parseExtraPeerData(fs, extraPeerDataFile, fileNumber);
if(!parseResult) {
gotError = true;
}
} catch (FSParseException e2) {
Logger.error(this, "Could not parse extra peer data: "+e2+ '\n' +fs.toString(),e2);
gotError = true;
}
return !gotError;
Copy link
Copy Markdown
Contributor

@Bombe Bombe Aug 27, 2024

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.

}

private SimpleFieldSet readFile(File extraPeerDataFile) {
FileInputStream fis;
try {
fis = new FileInputStream(extraPeerDataFile);
} catch (FileNotFoundException e1) {
Logger.normal(this, "Extra peer data file not found: "+extraPeerDataFile.getPath());
return false;
Logger.normal(this, "Extra peer data file not found: "+ extraPeerDataFile.getPath());
return null;
}
InputStreamReader isr = new InputStreamReader(fis, StandardCharsets.UTF_8);
BufferedReader br = new BufferedReader(isr);
Expand All @@ -531,25 +553,10 @@ public boolean readExtraPeerDataFile(File extraPeerDataFile, int fileNumber) {
try {
br.close();
} catch (IOException e5) {
Logger.error(this, "Ignoring "+e5+" caught reading "+extraPeerDataFile.getPath(), e5);
Logger.error(this, "Ignoring "+e5+" caught reading "+ extraPeerDataFile.getPath(), e5);
}
}
if(fs == null) {
Logger.normal(this, "Deleting corrupt (too short?) file: "+extraPeerDataFile);
deleteExtraPeerDataFile(fileNumber);
return true;
}
boolean parseResult = false;
try {
parseResult = parseExtraPeerData(fs, extraPeerDataFile, fileNumber);
if(!parseResult) {
gotError = true;
}
} catch (FSParseException e2) {
Logger.error(this, "Could not parse extra peer data: "+e2+ '\n' +fs.toString(),e2);
gotError = true;
}
return !gotError;
return fs;
}

private boolean parseExtraPeerData(SimpleFieldSet fs, File extraPeerDataFile, int fileNumber) throws FSParseException {
Expand Down