-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 FilterWindow #3364
Refactor FilterWindow #3364
Conversation
|
||
private List<String> readAsList(InputTextField field) { | ||
if (field.getText().isEmpty()) { | ||
return new ArrayList<>(); |
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.
Use Collections.emptyList()
instead of new ArrayList<>()
.
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.
Collections.emptyList()
returns an immutable list, so operations like add
will throw UnsupportedOperationException
. Are we sure that these lists are not modified anywhere ?
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.
I'm not 100% sure how the result is being used. You can run Bisq with this change and open filter editor and see if anything blows up.
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.
Ok, I changed into Collections.emptyList()
and did like you said - nothing wrong happened.
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.
ACK
@ripcurlx merge ? |
As this is a very sensitive view I want to test it myself before merging. But atm I'm focusing on the v1.2.0 release branch right now. Just something general: Refactorings are great, but it would be better to do them in the future while implementing feaures/fixing bugs with direct user benefit. As reviewing PRs is a bottleneck we need to be efficient and try to reduce review times as much as possible. |
@ripcurlx I'm trying to do small refactorings which eliminates code repetition, according to the Doing such a small refactorings help me to familiarize myself with the code. |
please resolve the conflicts so we can merge it |
@freimair Conflicts resolved |
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.
utAck. I did fire up the thing and created a filter. Everything works as I would expect it. @ripcurlx?
@lusarz This PR contains unsigned commits: https://help.github.com/en/github/authenticating-to-github/signing-commits |
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.
NACK - please sign your commits. Besides that ACK. Tested it by filtering two currencies on Regtest.
@ripcurlx Commit is singed now |
In this pull request I introduced
List<String> readAsList(InputTextField field)
function. It allowed me to remove a few repetitions of code.For example instead of:
I do:
Finally I was able to eliminate temporary variables like
nodes
,bannedCurrencies
etc, and invokereadAsList
directly.