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.

Sign up for GitHub

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

add factory pattern SimpleAtomicLongMultithreadedTest #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add factory pattern SimpleAtomicLongMultithreadedTest #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 48 additions & 36 deletions assignments/week-2-assignment-1/src/edu/vuum/mocca/SimpleAtomicLongMultithreadedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,6 @@ public class SimpleAtomicLongMultithreadedTest {
*/
static SimpleAtomicLong mCounter;

/**
* Runnable commands that use the mCounter methods
* get()
* incrementAndGet()
* getAndIncrement()
* decrementAndGet()
* getAndDecrement()
*/
static Runnable getCommand;
static Runnable incrementGetCommand;
static Runnable getIncrementCommand;
static Runnable decrementGetCommand;
static Runnable getDecrementCommand;

/**
* The value of mCounter prior to any changes made by testing.
*/
Expand Down Expand Up @@ -128,7 +114,43 @@ public void run() {
public long getIterations() {
return iterations;
}
}
}

/**
* @class RunnableFactory
*
* @brief This class provide method for create Runnable interface.
* Class demonstrates Factory pattern.
*/
static class RunnableFactory {

enum ETestCommand{
GET,
INCREMENT_AND_GET,
GET_AND_INCREMENT,
DECREMENT_AND_GET,
GET_AND_DECREMENT
}

static Runnable make(ETestCommand command){

switch (command){
Copy link

Choose a reason for hiding this comment

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

I think It's not the better way IMHO, check it please http://www.oodesign.com/factory-pattern.html

Copy link
Author

Choose a reason for hiding this comment

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

Can you explain me your position. I have read text at this link. But I do not see any problems in this implementation.

Copy link

Choose a reason for hiding this comment

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

I refer to this part "... This implementation is the most simple and intuitive (Let's call it noob implementation). The problem here is that once we add a new concrete product [Runnable] call we should modify the Factory class. It is not very flexible and it violates open close principle. Of course we can subclass the factory class, but let's not forget that the factory class is usually used as a singleton. Subclassing it means replacing all the factory class references everywhere through the code..."

To make my point clear:

  1. It would be more difficult add new tests, I would need to add a new enum , a case and a runnable implementation and call it, two steps more.
  2. I don't see any benefit.

Copy link
Author

Choose a reason for hiding this comment

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

I told in the discussions forum for mr. Douglas that may be in this case factory pattern is excess. And I just send pull request. Because I really think that it is more clearly and elegant solution than to have several static variables contains Runnable implementation.
What will you do if you will need to add more 10 test runnable implementations? You will add more 10 static variables. I think more flexible to store all implementation in one special place. And it will be easy to extract this class in own class file if it will be huge. It's about benefit.

About 1st point. Consider you want to add new test in realization with static vars. You still need to add new static var. Do you think it is really easier than add new enum item?

case INCREMENT_AND_GET:
return new Runnable() { public void run() { mCounter.incrementAndGet(); } };
case GET_AND_INCREMENT:
return new Runnable() { public void run() { mCounter.getAndIncrement(); } };
case DECREMENT_AND_GET:
return new Runnable() { public void run() { mCounter.decrementAndGet(); } };
case GET_AND_DECREMENT:
return new Runnable() { public void run() { mCounter.getAndDecrement(); } };
case GET:
default:
return new Runnable() { public void run() { mCounter.get(); } };
}

}

}

/**
* Runs prior to all tests. Creates a static instance of
Expand All @@ -140,17 +162,7 @@ public static void setUpBeforeClass() throws Exception {
* Instance of SimpleAtomicLong class
*/
mCounter = new SimpleAtomicLong(INITIAL_VALUE);

/**
* Runnable commands that execute get(), incrementAndGet(),
* getAndIncrement(), decrementAndGet(), getAndDecrement(),
* respectively, on the SimpleAtomicLong instance
*/
getCommand = new Runnable() { public void run() { mCounter.get(); } };
incrementGetCommand = new Runnable() { public void run() { mCounter.incrementAndGet(); } };
getIncrementCommand = new Runnable() { public void run() { mCounter.getAndIncrement(); } };
decrementGetCommand = new Runnable() { public void run() { mCounter.decrementAndGet(); } };
getDecrementCommand = new Runnable() { public void run() { mCounter.getAndDecrement(); } };

}

/**
Expand All @@ -169,7 +181,7 @@ public void multiGetTest() {
/**
* run multiple threads calling mCounter.get().
*/
runThreads(getCommand);
runThreads(RunnableFactory.make(RunnableFactory.ETestCommand.GET));
/**
* The expected post-test value is no change in the pre-test
* value.
Expand All @@ -184,7 +196,7 @@ public void multiGetTest() {
*/
@Test
public void multiIncrementAndGetTest() {
runThreads(incrementGetCommand);
runThreads(RunnableFactory.make(RunnableFactory.ETestCommand.INCREMENT_AND_GET));
/**
* expected value after threads are run should be the number
* of maximum iterations times the number of threads plus the
Expand All @@ -200,7 +212,7 @@ public void multiIncrementAndGetTest() {
*/
@Test
public void multiGetAndIncrementTest() {
runThreads(getIncrementCommand);
runThreads(RunnableFactory.make(RunnableFactory.ETestCommand.GET_AND_INCREMENT));
assertEquals(preTestValue
+ mMaxIterations*numThreads,
mCounter.get());
Expand All @@ -212,7 +224,7 @@ public void multiGetAndIncrementTest() {
*/
@Test
public void multiDecrementAndGetTest() {
runThreads(decrementGetCommand);
runThreads(RunnableFactory.make(RunnableFactory.ETestCommand.DECREMENT_AND_GET));
/**
* Expected value of mCounter after threads have completed
* running is the pre-test value minus the maximum iterations
Expand All @@ -228,7 +240,7 @@ public void multiDecrementAndGetTest() {
*/
@Test
public void multiGetAndDecrementTest() {
runThreads(getDecrementCommand);
runThreads(RunnableFactory.make(RunnableFactory.ETestCommand.GET_AND_DECREMENT));
/**
* expected value of mCounter after threads have completed running
* is the pre-test value minus the maximum iterations times the number
Expand Down Expand Up @@ -280,11 +292,11 @@ private void runThreads(Runnable command) {
RunTest[] runTests;
if(command == null) {
runTests = new RunTest[5];
runTests[0] = new RunTest(getCommand);
runTests[1] = new RunTest(incrementGetCommand);
runTests[2] = new RunTest(decrementGetCommand);
runTests[3] = new RunTest(getDecrementCommand);
runTests[4] = new RunTest(getIncrementCommand);
runTests[0] = new RunTest(RunnableFactory.make(RunnableFactory.ETestCommand.GET));
runTests[1] = new RunTest(RunnableFactory.make(RunnableFactory.ETestCommand.INCREMENT_AND_GET));
runTests[2] = new RunTest(RunnableFactory.make(RunnableFactory.ETestCommand.DECREMENT_AND_GET));
runTests[3] = new RunTest(RunnableFactory.make(RunnableFactory.ETestCommand.GET_AND_DECREMENT));
runTests[4] = new RunTest(RunnableFactory.make(RunnableFactory.ETestCommand.GET_AND_INCREMENT));
}
else {
runTests = new RunTest[numThreads];
Expand Down