Skip to content
This repository has been archived by the owner. It is now read-only.

[EAGLE-1012] Some language level problems in eagle-core module #925

Closed
wants to merge 5 commits into from
Closed

[EAGLE-1012] Some language level problems in eagle-core module #925

wants to merge 5 commits into from

Conversation

@asdf2014
Copy link
Member

@asdf2014 asdf2014 commented Apr 19, 2017

Some language level problems

  • Spell
  • Duplicated
  • Lambda
  • Collection
  • String
  • instanceof
  • Complex Method
  • Exception
  • AutoClose
  • Synchronized

Details see attachment files.

@asdf2014 asdf2014 changed the title [EAGLE-1012] Some language level problems [WIP][EAGLE-1012] Some language level problems Apr 19, 2017
@asdf2014 asdf2014 changed the title [WIP][EAGLE-1012] Some language level problems [EAGLE-1012] Some language level problems in eagle-core module Apr 19, 2017
@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented May 2, 2017

If these improvements are necessary, we should step by step merge to avoid too much code conflict. So, creating multiple sub PRs for EAGLE-1012 will be more suitable for this situation. What do you think? @jhsenjaliya @qingwen220

Loading

@@ -43,8 +43,6 @@ public static AttributeResolvable getAttributeResolver(String fieldResolverName,
fieldResolvableCache.put(fieldResolverName, instance);
} catch (ClassNotFoundException e) {
throw new AttributeResolveException("Attribute Resolver in type of " + fieldResolverName + " is not found", e);
} catch (InstantiationException | IllegalAccessException e) {
Copy link
Contributor

@jhsenjaliya jhsenjaliya May 2, 2017

Choose a reason for hiding this comment

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

instead of removing this, it can be used to describe the error, IDE is suggesting to collapse the catch block may be because it has identical error msg (throw new AttributeResolveException(e)).

Loading

Copy link
Member Author

@asdf2014 asdf2014 May 2, 2017

Choose a reason for hiding this comment

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

I see. I will roll it back.

Loading

for (AlertStreamEvent e : eventList) {
alertEvents.add(AlertPublishEvent.createAlertPublishEvent(e));
}
List<AlertPublishEvent> alertEvents = eventList.stream().map(
Copy link
Contributor

@jhsenjaliya jhsenjaliya May 2, 2017

Choose a reason for hiding this comment

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

i feel its fancy to use newer java features, but old syntax is more readable, may be its just a coding preference

Loading

Copy link
Member Author

@asdf2014 asdf2014 May 2, 2017

Choose a reason for hiding this comment

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

I think it will bring better maintainability and performance.

Loading

Copy link
Member Author

@asdf2014 asdf2014 May 2, 2017

Choose a reason for hiding this comment

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

Maybe you are right, i will roll it back. 👍

Loading

Copy link
Member Author

@asdf2014 asdf2014 May 2, 2017

Choose a reason for hiding this comment

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

I have created a non-professional benchmark test. After adding -Xmx2G -Xms2G -Xmn1G -XX:+AlwaysPreTouch params for reducing the impact from jvm resources, then found the performance gap is not very large, and the performance of lambda stream is better than simple for loop after the data becomes larger.

Loading

@@ -199,7 +194,7 @@ private void closePlugins(List<AlertPublishPlugin> toBeClosed) {
try {
p.close();
} catch (Exception e) {
LOG.error(String.format("Error when close publish plugin {}!", p.getClass().getCanonicalName()), e);
LOG.error(String.format("Error when close publish plugin %s!", p.getClass().getCanonicalName()), e);
Copy link
Contributor

@jhsenjaliya jhsenjaliya May 2, 2017

Choose a reason for hiding this comment

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

people are using this interchangeably

Loading

Copy link
Member Author

@asdf2014 asdf2014 May 2, 2017

Choose a reason for hiding this comment

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

String.format should use %s rather than {}

@Test
public void testFormat() {
    assertEquals("a{}c", String.format("a{}c", "b"));
    assertEquals("abc", String.format("a%sc", "b"));
}

Loading

@jhsenjaliya
Copy link
Contributor

@jhsenjaliya jhsenjaliya commented May 2, 2017

@asdf2014 , i havent gone through all the changes yet, but looks like these are what some particular IDE would recommend, some are real improvements as well. lets check this out after 0.5, Thanks for the efforts !

Loading

@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented May 2, 2017

@jhsenjaliya Okay, i see. Thx for your code review.

Loading

@jhsenjaliya
Copy link
Contributor

@jhsenjaliya jhsenjaliya commented May 2, 2017

@asdf2014 Thanks again, lets work on this after 0.5

Loading

@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented May 3, 2017

@jhsenjaliya You are welcome. Sorry, i just note it. Let’s finish this after v0.5.0 together.

Loading

@asdf2014 asdf2014 closed this Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants