-
-
Notifications
You must be signed in to change notification settings - Fork 503
106 hello thread exercise demo #114
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
Conversation
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.
@tkuzub please take a look at my comments. Thank you!
pom.xml
Outdated
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.
- please move this line up (right after the 6th module)
- please remove the next line (you should follow the same approach we have), in other words, this pom file declares only high-level modules. And
7-0-0-hello-threadsshould be declared as a submodule of7-0-java-concurrencyinside its pom file.
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.
@tkuzub there is a mistake here: it should be 7-0-0-hello-threads not 7-0-0-hello-theads
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.
@tkuzub a class should have a javadoc as well. For instance
/**
* {@link HelloThreads} provide a simple API for working with Java threads. A {@link Thread} class represents a key
* concept in Java Concurrency. So this class is meant to teach you the basics like creating a thread or checking its
* state.
* <p>
* todo: implement each method according to javadoc and verify your impl by running {@link HelloThreadsTest}
*
* @author Taras Kuzub
*/
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.
@tkuzub let's better add another parameter to specify the number of threads.
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.
@tkuzub I don't like this approach with a shared queue. I guess it's better that you create a new instance of a queue in each test if you need and, then use it, and then verify what you want to verify.
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.
It should not assert the queue content, since this method is only created to the thread name.
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.
It should not assert the queue content.
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.
@tkuzub every test should follow the patter
- build – where you prepare everything you need
- operate - where you perform the action that you want to test
- assert - where you verify the results and check if they are correct
Another similar pattern is given, when, then.
For the assertions we use AsserJ. So for example the following test should look like this:
@SneakyThrows @Test @Order(3) void getThreadState() { var thread = new Thread(waitASecond()); thread.start(); var state = HelloThreads.getThreadState(thread); assertThat(state).isEqualTo(RUNNABLE); }
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.
@tkuzub please move all the code to package com.bobocode.concurrency
@tkuzub it looks like there is no progress on this task. I'm going to close this PR if you don't mind.
@tboychuk, Ok
No description provided.