Skip to content

Module pattern #62 #512

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

Merged
merged 8 commits into from
Dec 1, 2016
Merged

Module pattern #62 #512

merged 8 commits into from
Dec 1, 2016

Conversation

inbravo
Copy link
Contributor

@inbravo inbravo commented Oct 27, 2016

Pull request for Module pattern #62

FirstCut++
SecondCut++
App++
App
checkstyle errors removed
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 83.657% when pulling ea7752c on inbravo:master into 19cb715 on iluwatar:master.

@inbravo inbravo mentioned this pull request Oct 27, 2016
@iluwatar iluwatar self-assigned this Nov 14, 2016
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.

@inbravo looks good in general. I added some review comments. Please comment when you're done with the changes and we can proceed with merge.

@@ -0,0 +1,95 @@
/**
* The MIT License Copyright (c) 2016 Amit Dixit
Copy link
Owner

Choose a reason for hiding this comment

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

Please retain the license header consistent e.g. The MIT License Copyright (c) 2014 Ilkka Seppälä

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

public final class App {

public static FileLoggerModule fileLoggerModule = null;
public static ConsoleLoggerModule consoleLoggerModule = null;
Copy link
Owner

Choose a reason for hiding this comment

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

These are initialized to null automatically. Check for similar occurrences elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


/* Prepare modules */
fileLoggerModule.prepare();
consoleLoggerModule.prepare();
Copy link
Owner

Choose a reason for hiding this comment

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

It would be possible to create the object and prepare it on one line

Copy link
Contributor Author

@inbravo inbravo Nov 15, 2016

Choose a reason for hiding this comment

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

Method prepare() is independent to demonstrate modular aspect of pattern. Please clarify.

* the creation and organization of other elements, and groups them as the structural pattern does.
* An object that applies this pattern can provide the equivalent of a namespace, providing the
* initialization and finalization process of a static class or a class with static members with
* cleaner, more concise syntax and semantics.
Copy link
Owner

Choose a reason for hiding this comment

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

I would not duplicate this comment everywhere. It could be only in App.java.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

* @throws IOException if program is not able to find log files (output.txt and error.txt)
*/
@Test
public void positiveTestFileMessage() throws IOException {
Copy link
Owner

Choose a reason for hiding this comment

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

I would reconsider JUnit test method naming. See the other patterns for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@iluwatar
Copy link
Owner

@inbravo is this ready for another review?

@inbravo
Copy link
Contributor Author

inbravo commented Nov 29, 2016

Your third comment on module/src/main/java/com/iluwatar/module/App.java, require clarification. Kindly advise.

@iluwatar
Copy link
Owner

@inbravo yes, so instead of this

/* Create new singleton objects and prepare their modules */
fileLoggerModule = FileLoggerModule.getSingleton();
consoleLoggerModule = ConsoleLoggerModule.getSingleton();

/* Prepare modules */
fileLoggerModule.prepare();
consoleLoggerModule.prepare();

You could write

FileLoggerModule.getSingleton().prepare();
ConsoleLoggerModule.getSingleton().prepare();

@inbravo
Copy link
Contributor Author

inbravo commented Dec 1, 2016

Code is Done! as per all review comments .....

@iluwatar iluwatar merged commit c94c8a3 into iluwatar:master Dec 1, 2016
@iluwatar
Copy link
Owner

iluwatar commented Dec 1, 2016

Well done @inbravo 👍 Thank you for the new pattern!

@iluwatar
Copy link
Owner

@all-contributors please add @inbravo for code

@allcontributors
Copy link
Contributor

@iluwatar

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

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.

3 participants