Skip to content
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

PAYARA-4015 Module admingui/common-console replaced synchronized classes... #4096

Conversation

svendiedrichsen
Copy link
Contributor

@svendiedrichsen svendiedrichsen commented Jul 18, 2019

... with unsynchronized. extracted a method for code copy.

Apologies for the reformatting of one of the classes. The formatting was way off.

The replacement of the used Stack class in the inner class JsonChars is thread-safe because every call creates a new instance of the JsonChars class. Thus only a single thread will enter it.

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @svendiedrichsen for the PR, only a few minor naming issues and suggestions.

private static String[] getSelectedCiphersList(String selectedCiphers){
List<String> selItems = new ArrayList<>();
if(selectedCiphers != null){
String[] sel = selectedCiphers.split(","); //NOI18N
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid short names or abbreviations for variable names. Maybe this can just be inlined in the for loop?

Vector ciphersVector = null;
Object ciphers = (Object)handlerCtx.getInputValue("ciphers");
List<String> ciphersList = null;
Object ciphers = handlerCtx.getInputValue("ciphers");
if (ciphers != null) {
if (ciphers instanceof String) {
String[] ary = getSelectedCiphersList((String) ciphers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline to avoid short variable name?

return sb.toString();
}

private static List<String> getCiphers(String[] allCiphers){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this method be replaced with Arrays.asList(allCiphers) or does the resulting List have to be modifiable?

}
return listCiphers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all methods using filterCiphers convert the returned list to an array directly maybe that common part could be done here as well?

return filterCiphers(ciphers, BIT_CIPHERS).toArray(new String[0]);
}

private static List<String> filterCiphers(List<String> ciphers, Set<String> filterList){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If filterCiphers would take a Predicate<String> evaluating if a cipher is accepted all the filters could be done with the same function (breakUpCiphers could also use filter) and in one pass (breakUpCiphers would not needed to be called multiple times) and the code would be more readable formulating the condition of the ciphers to keep as a lambda. WDYT?

@svendiedrichsen
Copy link
Contributor Author

@jbee I have made the requested changes.

@Pandrex247 Pandrex247 changed the title Module admingui/common-console replaced synchronized classes... PAYARA-4015 Module admingui/common-console replaced synchronized classes... Jul 26, 2019
@Pandrex247
Copy link
Member

Jenkins test please

@jbee jbee merged commit 6e73c4c into payara:master Jul 26, 2019
@svendiedrichsen svendiedrichsen deleted the admingui-console-common-synchronized-classes branch July 26, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants