BAEL-1889 - Let's move the Java Number articles into a new module#4619
BAEL-1889 - Let's move the Java Number articles into a new module#4619pivovarit merged 11 commits intoeugenp:masterfrom JonCook:master
Conversation
jzheaux
left a comment
There was a problem hiding this comment.
@JonCook hey, nice job.
Just a couple of small items in general
- Double-check the whitespacing. Indentation in the pom is wonky--it's probably from your editor, but just see if you can clean that up so the XML comes out nice.
- Unless you heard differently from Eugen, let's leave test improvements out of this PR. Feel free to log an enhancement for improving the tests, etc. (I've left some comments with more details about this.)
| <artifactId>jmh-generator-annprocess</artifactId> | ||
| <version>${jmh-generator-annprocess.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Double-check some of the whitespacing here. Looks like these few dependencies don't have the same indentation as the reast.
| <groupId>ch.qos.logback</groupId> | ||
| <artifactId>logback-classic</artifactId> | ||
| <version>${logback.version}</version> | ||
| <!-- <scope>runtime</scope> --> |
There was a problem hiding this comment.
Is there a reason this comment-out needs to be here (like for an article that references it?). If not, let's clean it up.
| </dependency> | ||
| </dependencies> | ||
|
|
||
| <build> |
There was a problem hiding this comment.
The indentation looks off here. Note the closing one and how </plugin> is more indented than <plugin> is here under <builld>
|
|
||
| <properties> | ||
| <commons-math3.version>3.6.1</commons-math3.version> | ||
| <decimal4j.version>1.0.3</decimal4j.version> |
| private static Logger LOG = Logger.getLogger(NumberOfDigitsDriver.class); | ||
|
|
There was a problem hiding this comment.
Hmm, while I agree that slf4j is totally the way to go, I think this task should just be about moving the classes over. Would you be okay with moving this back to log4j and logging an enhancement?
| LOG.info("String Based Solution : {}", +length); | ||
|
|
||
| length = numberOfDigits.logarithmicApproach(602); | ||
| LOG.info("Logarithmic Approach : " + length); |
There was a problem hiding this comment.
Same comment about just moving the tests and not improving their contents.
| @@ -1,16 +1,14 @@ | |||
| package com.baeldung.algorithms.primechecker; | |||
| package com.baeldung.prime; | |||
There was a problem hiding this comment.
What is your reasoning for the package name change?
If it is something like "readability" or "cosmetics", then let's try and avoid this for this improvement. (While I do think these changes are great, they can be done in a different ticket)
|
Thanks for the comments. I will resubmit the PR. |
|
@jzheaux - Thanks for the comments! I updated the PR. I'm not sure what happened with the formatting in the pom.xml but I think it should be OK now. Probably a tab setting in my xml editor in eclipse. Sorry for including additional improvements that weren't part of the ticket. It is just the way I work, to clean often, frequently and in small incrementations. As I was creating a new module I thought it was a good opportunity to do this following the recommendations in the dev docs. But of course happy if this is done under a different improvement. But the tendency is this type of cleaning/improvements tend to get forgotten ;-) I reverted to log4j and put the original package names back. Thanks again for the comments. |
|
@JonCook The issue is that associated code in the articles also needs to be edited as part of that. Changing package names is one thing since those don't really end up in the article code samples, but changing more substantial things is a slippery slope that can end in confusing readers who see one thing in the article and another in the codebase. Feel free to ping Jira Support about adding another enhancement task to update articles to slf4j (etc.), and it's possible they will say that it is easier to do in this PR--it just means you'd be editing the contents of those articles, too. |
|
@jzheaux - Thanks for the comments again ;-) I'm still learning and quite new so agree with your suggestions. I don't think the formatting changes should have much impact on the articles. I had a quick look and most of it seems to be the same. Thanks again. |
No description provided.