Dirty Flag pattern #560 #717
Conversation
Bringing fork up-to-date
|
@iluwatar I will pick up this review. |
|
@iluwatar Done. |
|
Seems to be a JUnit test failure:- dummyCustomerApiTest() Time elapsed: 0.052 sec <<< FAILURE! Doesn't seem to be related to my changes here. |
|
It is a known issue #643, still unresolved |
|
@npathai please review the PR |
|
@iluwatar Yes started. Will complete today |
| return data; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
I am not sure if DataFetcher should be returning null to indicate no change in data. Any other options for this?
| * @return World instance | ||
| */ | ||
| public static World getInstance() { | ||
| if (world == null) { |
There was a problem hiding this comment.
Suggestion: Just document a warning that this is not a thread safe way to implement Singleton. Refer to Singleton for more information.
| * | ||
| * @return DataFetcher instance | ||
| */ | ||
| public static DataFetcher getInstance() { |
There was a problem hiding this comment.
Question: I don't understand the intent to keep World and DataFetcher singleton here. Any particular reason you chose this way?
|
|
||
| @BeforeEach | ||
| public void reset() throws SecurityException, NoSuchFieldException, IllegalArgumentException, IllegalAccessException { | ||
| Field instance = DataFetcher.class.getDeclaredField("df"); |
There was a problem hiding this comment.
This singleton reset thing won't be needed if we avoid getInstance. Hence my question about Singletonness.
| To avoid expensive re-acquisition of resources. The resources retain their identity, are kept in some | ||
| fast-access storage, and are re-used to avoid having to acquire them again. | ||
|
|
||
|  |
There was a problem hiding this comment.
The class diagram should be made with ObjectAid eclipse plugin. More about it in wiki
|
@waisuan You have my review comments. Overall it looks great and easy to follow. 👍 There are a few points that I have highlighted though. Awaiting your response. |
|
@waisuan Any updates? |
|
@npathai Apologies for the delay here. Been swamped. I'll take a look at your comments tonight and begin working on them accordingly. Cheers! |
|
@npathai -- Fixes are up. Could you take another peek? Thanks! |
|
@waisuan 👍 Surely. I will do that tomorrow. |
| public void testIsDirty() { | ||
| DataFetcher df = new DataFetcher(); | ||
| List<String> countries = df.fetch(); | ||
| assertTrue(!countries.isEmpty()); |
There was a problem hiding this comment.
Use assertFalse() in place of assertTrue(!condition)
Uh oh!
There was an error while loading. Please reload this page.