Issue #469: Event-based Asynchronous pattern by waisuan · Pull Request #483 · iluwatar/java-design-patterns

waisuan · 2016-08-08T22:35:58Z

Hey @iluwatar . Let me know if there are any issues with the implementation of the new pattern. Thanks.

coveralls · 2016-08-08T23:01:28Z

Coverage decreased (-0.1%) to 88.924% when pulling ca30b2e on waisuan:master into d50139e on iluwatar:master.

waisuan · 2016-08-08T23:17:33Z

Failed at Checkstyle. Working on it.

waisuan · 2016-08-09T08:55:39Z

Issue seems to be stemming from PMD check. I believe that it is due to the unnecessary "public" scope in IEvent. Will submit a fix before the end of today.

coveralls · 2016-08-09T21:33:50Z

Coverage decreased (-1.1%) to 87.928% when pulling d9bfb6d on waisuan:master into d50139e on iluwatar:master.

waisuan · 2016-08-09T21:37:35Z

Coverage decreased by ~1%. Is it still OK to merge?

waisuan · 2016-08-16T22:37:17Z

Hey, @iluwatar . Any thoughts on this so far? :)

iluwatar · 2016-09-03T10:08:28Z

@waisuan My apologies for taking this long to review the pull request. Here are my comments:

Can we add some spice to the example? I mean now it starts and stops some generic events which is technically ok, but I would like to add some human elements also. How about using a microwave oven, boiling eggs or operating a time machine?
I like the interactive mode. However, if there are events running the application does not seem to exit. Is this a bug or feature? Maybe it should output some info like "cannot exit while events are running"?
In interactive mode it is fun to play with events. However, the event ids are quite long and hard to type. Could we use shorter event ids?
The description of the pattern gives quite strict rules for naming the methods such as Load, LoadAsync and LoadCompleted. Should we use this naming pattern also?
There are no assertions in the unit tests, they just test that exceptions are not thrown. Could we add some assertions?

iluwatar · 2016-09-03T10:10:01Z

+permalink: /patterns/event-asynchronous/
+categories: Other
+tags:
+ - Java


Can you think proper categories and tags for the pattern? For reference see http://java-design-patterns.com/patterns/

iluwatar · 2016-09-03T10:19:57Z

@waisuan You have my review comments. Please comment on this thread once you're ready for another review.

coveralls · 2016-09-03T10:54:00Z

Coverage decreased (-0.9%) to 88.058% when pulling 233f1e6 on waisuan:master into ff23e90 on iluwatar:master.

waisuan · 2016-09-05T08:11:24Z

Thanks for getting back to me, @iluwatar . Your comments have been noted. I'll try to get to them sometime this week. Cheers.

coveralls · 2016-09-11T19:59:25Z

Coverage decreased (-1.3%) to 84.692% when pulling f215951 on waisuan:master into 59e6a0a on iluwatar:master.

waisuan · 2016-09-11T20:01:29Z

Hey, @iluwatar . I've made the necessary changes based on your feedback. Do take another look and let me know if there is anything else. Thanks!

iluwatar · 2016-09-17T19:24:16Z

@waisuan here are my comments:

I could not trigger the interactive mode using the properties file.
The interactive mode menu is confusing. Especially 3 and 4 seem to do the same thing. Can we combine these into one?
The interactive mode application is somewhat unstable. See the following console log:

Hello. Would you like to boil some eggs?
(1) BOIL AN EGG 
(2) STOP BOILING THIS EGG 
(3) HOW IS MY EGG? 
(4) HOW ARE MY EGGS? 
(5) EXIT
Choose [1,2,3,4,5]: 1
Boil multiple eggs at once (A) or boil them one-by-one (S)?: S
How long should this egg be boiled for (in seconds)?: 5
Egg [986] is being boiled.
Hello. Would you like to boil some eggs?
(1) BOIL AN EGG 
(2) STOP BOILING THIS EGG 
(3) HOW IS MY EGG? 
(4) HOW ARE MY EGGS? 
(5) EXIT
Choose [1,2,3,4,5]: 1
Boil multiple eggs at once (A) or boil them one-by-one (S)?: S
How long should this egg be boiled for (in seconds)?: [986] is done.
5
Event [986] is still running. Please wait until it finishes and try again.
Hello. Would you like to boil some eggs?
(1) BOIL AN EGG 
(2) STOP BOILING THIS EGG 
(3) HOW IS MY EGG? 
(4) HOW ARE MY EGGS? 
(5) EXIT
Choose [1,2,3,4,5]: 5
Exception in thread "main" java.lang.NullPointerException
    at com.iluwatar.event.asynchronous.Event.stop(Event.java:45)
    at com.iluwatar.event.asynchronous.EventManager.shutdown(EventManager.java:172)
    at com.iluwatar.event.asynchronous.App.runInteractiveMode(App.java:194)
    at com.iluwatar.event.asynchronous.App.run(App.java:94)
    at com.iluwatar.event.asynchronous.App.main(App.java:64)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)

Process finished with exit code 1

I suggest you add more unit tests to catch errors. Try for example running different combinations of events, starting & stopping & restarting etc.

coveralls · 2016-09-19T21:15:31Z

Coverage decreased (-1.6%) to 82.756% when pulling 69cab15 on waisuan:master into 6026eed on iluwatar:master.

waisuan · 2016-09-19T21:19:53Z

Hey, @iluwatar . Kindly take another look at my changes. The properties file accept "YES" or "NO" as its input. I've toggled the settings. They seem to work just fine. I've fine-tuned the interactive mode as well. It was meant to be a "fun" way of displaying the design pattern at hand; not take focus away from it. I can remove it if you like. I prefer not to complicate the interactive mode any further so that the focus on this design pattern can be the pattern itself. :)

iluwatar · 2016-09-20T19:44:16Z

+title: Event-based Asynchronous
+folder: event-asynchronous
+permalink: /patterns/event-asynchronous/
+categories: Other


We should use Concurrency category

iluwatar · 2016-09-20T19:48:07Z

+      System.out.println("Event [" + aEventId + "] has been stopped.");
+      eventManager.cancel(sEventId);
+      System.out.println("Event [" + sEventId + "] has been stopped.");
+


Can we change the output so that it tells whether we are talking about sync or async event. For example "Async event [xxx] has been created" and "Sync event [xxx] has been created"?

iluwatar · 2016-09-20T19:54:05Z

+    } catch (MaxNumOfEventsAllowedException | LongRunningEventException | EventDoesNotExistException e) {
+      System.out.println(e.getMessage());
+    }
+  }


We should add tests to run operations from start to finish also. Create and start event and wait for it to finish. For async operations we could create multiple events and wait for all of them to finish.

iluwatar · 2016-09-20T19:54:51Z

+    long endTime = currentTime + (eventTime * 1000);
+    while (System.currentTimeMillis() < endTime) {
+      try {
+        Thread.sleep(5000); // Sleep for 5 seconds.


I think the granularity should be 1 seconds instead of 5 seconds here.

waisuan · 2016-09-27T22:05:24Z

Hey, @iluwatar . Your feedback has been noted. I'll get to them sometime this week.

waisuan · 2016-10-03T13:47:42Z

Gonna work on this today. :)

coveralls · 2016-10-03T21:13:45Z

Coverage decreased (-1.02%) to 83.273% when pulling 8f1758c on waisuan:master into 4ca205c on iluwatar:master.

waisuan · 2016-10-03T21:14:22Z

Done, @iluwatar

iluwatar

@waisuan the interactive mode cannot be triggered with a change to config.properties file. I changed the property to INTERACTIVEMODE=YES and ran the program in IntelliJ Idea. I also tried placing the config.properties file to the same folder as the produced event-asynchronous-1.14.0-SNAPSHOT.jar and running the jar with java -cp event-asynchronous-1.14.0-SNAPSHOT.jar com.iluwatar.event.asynchronous.App. The app still runs in non-interactive mode. Am I missing something or how does it work for you?

I propose you put the config.properties in src/main/resources to fix this issue.

coveralls · 2016-10-17T21:44:57Z

Coverage decreased (-0.8%) to 83.531% when pulling 7031812 on waisuan:master into 4ca205c on iluwatar:master.

waisuan · 2016-10-17T21:45:42Z

Not sure what happened there, @iluwatar . It works on Eclipse. Regardless, I've moved the config file into a separate directory as suggested.

iluwatar · 2016-10-18T04:44:03Z

@waisuan work well done! Thank you for the new pattern contribution 👍

iluwatar added the status: under review label Sep 3, 2016

iluwatar self-assigned this Sep 3, 2016

iluwatar reviewed Sep 3, 2016
View reviewed changes

WSSIA added 4 commits September 3, 2016 11:32

Issue #469: Implementation of Event-based Asynchronous pattern

2d99061

Updated parent POM to include new pattern (Event-asynchronous)

e1836fe

Fixed Checkstyle errors.

f115971

Removed PUBLIC modifiers from IEvent

233f1e6

WSSIA added 3 commits September 11, 2016 16:24

Updating fork branch

fce30db

Changes based on code review

9a90f2d

Merge remote-tracking branch 'upstream/master'

f215951

WSSIA added 3 commits September 19, 2016 21:50

Changes based on latest code review

dbd605e

Changes based on latest code review

371b262

Merge remote-tracking branch 'upstream/master'

69cab15

iluwatar requested changes Sep 20, 2016

View reviewed changes

WSSIA added 4 commits October 3, 2016 21:05

Changes based on review feedback.

12544ca

Merge remote-tracking branch 'upstream/master'

5eacf52

Updated version snapshot to 1.14.0

622376e

Alter JUnit tests to run in lesser time.

8f1758c

iluwatar requested changes Oct 16, 2016

View reviewed changes

WSSIA added 2 commits October 17, 2016 21:29

Moved config into a separate dir

986c529

Changed config to non-interactive

7031812

iluwatar approved these changes Oct 18, 2016

View reviewed changes

iluwatar merged commit 0f7b44c into iluwatar:master Oct 18, 2016

Uh oh!

Conversation

waisuan commented Aug 8, 2016

Uh oh!

coveralls commented Aug 8, 2016

Uh oh!

waisuan commented Aug 8, 2016

Uh oh!

waisuan commented Aug 9, 2016

Uh oh!

coveralls commented Aug 9, 2016

Uh oh!

waisuan commented Aug 9, 2016

Uh oh!

waisuan commented Aug 16, 2016

Uh oh!

iluwatar commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluwatar Sep 3, 2016

Choose a reason for hiding this comment

Uh oh!

iluwatar commented Sep 3, 2016

Uh oh!

coveralls commented Sep 3, 2016

Uh oh!

waisuan commented Sep 5, 2016

Uh oh!

coveralls commented Sep 11, 2016

Uh oh!

waisuan commented Sep 11, 2016

Uh oh!

iluwatar commented Sep 17, 2016

Uh oh!

coveralls commented Sep 19, 2016

Uh oh!

waisuan commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluwatar Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

iluwatar Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

iluwatar Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

iluwatar Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

waisuan commented Sep 27, 2016

Uh oh!

waisuan commented Oct 3, 2016

Uh oh!

coveralls commented Oct 3, 2016

Uh oh!

waisuan commented Oct 3, 2016

Uh oh!

iluwatar left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 17, 2016

Uh oh!

waisuan commented Oct 17, 2016

Uh oh!

iluwatar commented Oct 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iluwatar commented Sep 3, 2016 •

edited

Loading

waisuan commented Sep 19, 2016 •

edited

Loading