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.

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

Code style issues fixed #266

Merged
merged 3 commits into from
Jul 26, 2016
Merged

Conversation

krasa
Copy link
Contributor

@krasa krasa commented Jul 25, 2016

  • explicit super() call, introduced constants from magic strings, moved constants to one place.

…magic strings, moved constants to one place.

public class Constants {

public static final String AN_ATTEMPT_WAS_MADE_TO_DIVIDE_BY_ZERO = "An attempt was made to divide by zero.";

Choose a reason for hiding this comment

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

Shouldn't this use DI so that we can change it at any time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing after another. We could inject those values in the future.

Choose a reason for hiding this comment

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

make sure to send an email to all of our mailing lists and to each developers' managers

@emiln
Copy link
Member

emiln commented Jul 26, 2016

I think this is a very appealing changeset. My one concern is about the many constants residing inside the FizzBuzzTest class. I think it might be worth moving them to a class of constants used for testing. Perhaps simply a Constants class inside the test code, or even TestConstants if we fear ambiguity. With "Testing" being a must win for the company this year, I expect we'll see a regular plethora of additional tests as soon as our QA and Dev teams get started. I imagine we'll then come to regret not having a concise and separate class for just these kinds of constants.

@krasa
Copy link
Contributor Author

krasa commented Jul 26, 2016

Excellent point, will do.

@emiln emiln merged commit 051fb7e into EnterpriseQualityCoding:master Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants