Skip to content

Conversation

thomasoss
Copy link
Contributor

Example for using a ThreadLocal variable in a Runnable object. To show the problems when not using the Thread Local Storage pattern a second app named AppUgly is added. This second app does not use the Thread Local Storage pattern and produces exeception and / or bad results. For AppUgly no JUnit tests are provided because these would neither surely fail nor surley succeed. Both apps are described in the code (javadoc).
I could not find a way to check the rendering of the READMD.md.

@iluwatar iluwatar self-assigned this Dec 24, 2016
@iluwatar iluwatar self-requested a review December 24, 2016 08:20
@iluwatar
Copy link
Owner

Here are my initial review comments:

  • tls module is missing from the parent pom.xml.
  • The example is not very meaningful at the moment. Could we think of an example with a story to make it more interesting? See the other patterns for examples.
  • Use of plain Threads is kind of an antipattern. Prefer using ExecutorService instead.
  • I would leave out the ugly example. The consequences could just be explained. Now it is kind of copy-paste of the main example.
  • I would separate the AppTest and other tests to make it uniform with the other patterns.

@thomasoss please comment on this thread once the comments have been addressed.

@thomasoss
Copy link
Contributor Author

Thank you for your comments. I'm a little bit busy at the moment but I will deal with them within the next weeks. Have good start for 2017.

Some additonal description, deleted wrong pumlid
no change of content. Improved text
new uml diagramm after changes to the java classes
Rework replaces previous version completely. Using ExecutorService. Use of result object instead of static variables. Ugly example is left out.
Test reworked completely. AppTest seperated.
added  <module>tls</module>
removed errors caused by copy code from master
@thomasoss
Copy link
Contributor Author

Hello

  • tls is added to the parent pom.xml. I copied the code of the current version of pom.xml the 29th January 2017. Despite this beside the addition of tls two other changes are shown in the pull request. The line with the version and some eof marker. Maybe I'm a little bit unexperienced using github.
  • The example is unchanged. I don't have time at the moment to change it to another example. I can do it not before April because of a 3 week holiday trip and a 2 week business trip until the end of March.
  • ExecutorService is used now
  • ugly example is left out. For testing the problems arising with not using Thread Local Storage interested persons can comment out and comment in some marked pieces of DateFormatCallable
  • AppTest is separated

The pumlid I created last time was not correct. This time no pumlid is enclosed due to your proposal in december regarding the pumlid

tls/pom.xml Outdated
<parent>
<groupId>com.iluwatar</groupId>
<artifactId>java-design-patterns</artifactId>
<version>1.14.0-SNAPSHOT</version>
Copy link
Owner

Choose a reason for hiding this comment

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

Should be 1.15.0-SNAPSHOT

Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

The build is failing at the moment. Also, lots of commented out code detected.

Changed
    <version>1.14.0-SNAPSHOT</version>
to
    <version>1.15.0-SNAPSHOT</version>
@thomasoss
Copy link
Contributor Author

The code commented out is by purpose to allow running the example without thread local storage and see the results. Please view it. But I can remove it of course.
I changed to 1.15.0-SNAPSHOT in the tls/pom.xml. I could verify the build failure with the current version of the parent pom.xml und the "1.14-SNAPSHOT" entry in the tls/pom.xml in my local environment. After changing it to 1.15 the build was successful in my local environment. I hope it will do in the target environment too.

@iluwatar
Copy link
Owner

@thomasoss the build is still failing. You can see the build log by clicking the Details next to Travis icon.

Correction of error detected by maven-pmd-plugin.
Correction of correction ;-)
@thomasoss
Copy link
Contributor Author

@iluwatar I could reproduce the fail. I used the same mvn call as shown in the build log (mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V -e). I corrected the error found by the maven-pmd-plugin and in my environment the maven build is now successful. I hope it will do so in the target environment.

@iluwatar iluwatar merged commit 854101b into iluwatar:master Apr 1, 2017
@iluwatar
Copy link
Owner

iluwatar commented Apr 1, 2017

@thomasoss this looks good. Thank you for the contribution 👍

@iluwatar iluwatar added this to the 1.15.0 milestone Apr 1, 2017
@iluwatar
Copy link
Owner

@all-contributors please add @thomasoss for code

@allcontributors
Copy link
Contributor

@iluwatar

I've put up a pull request to add @thomasoss! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants