Correct checkstyle fixes (except TODOs) #54
Conversation
Fixed the formatting in ImgProvider.java and ImageLab.java to fit appropriate amount of characters per string Slight formatting fix Fixed all checkstyle issues on ImgProvider.java Fixed checkstyle errors for three classes added setter method to ILFrame class Correct code convention violations in Run.java * Fixed checkstyle issues with 'impro' variable in ImageLab.java * Removed intellij files * Fixed small issue with image provider variable not being set properly in ImgProvider.java
|
Removed a stray space from |
|
Was this closed in order to open a new (clean) PR? |
|
Apologies, I had the wrong assumption about "close" when I published the comment. |
| public static void main(String[] args) { | ||
| public final class Run { | ||
| /** | ||
| * Default constructor - not used. |
There was a problem hiding this comment.
One you add an explicit constructor, Java no longer creates a "default constructor".
Perhaps a more appropriate comment would be "Hide the default constructor for this utility class."
| * @param args Command-line arguments; passed into the main | ||
| * application |
There was a problem hiding this comment.
To be more useful, this comment should answer the question: "What are the command-line arguments to ImageLab?"
| @@ -19,7 +19,6 @@ | |||
| private Image img; | |||
| /** The display panel of this frame. */ | |||
| private DisPanel pane; | |||
|
|
|||
There was a problem hiding this comment.
What is the rationale for removing this whitespace?
Looking at this block of code, it seems like more (rather than less) whitespace might be useful.
There was a problem hiding this comment.
As I try to review this, I'm finding the PR too broad and encompasses a breadth of change types, from simple formatting to significant refactoring and associated design decision making.
The original issue (#32), identified as an epic, doesn't appropriately reflect the complexity of the objective. Although there is a single tool that is identifying shortcomings in the source code, Checkstyle, the reported "errors" are not all of the same type, do not require the same degree of design choice, nor do they have the same level of impact on the maintainability of the product.
This PR needs to be broken into smaller unit PRs.
The sub-issues (#33, #34, #35) likewise may each need to be broken down into smaller sub-issues to enable meaningful corrections in shorter time cycles.
I'm leaving in the few comments I'd made already, but they by no means constitute a full review.
Will set this PR to DRAFT with the expectation that it will be CLOSED after further discussion and potential reorganization of the User Stories.
| @@ -32,4 +32,13 @@ public void setActive() { | |||
| public void byebye() { | |||
| improvider.setInactive(); | |||
| } | |||
| /** @return ImgProvider object. */ | |||
There was a problem hiding this comment.
A more meaningful comment would be appreciated. (This just reiterates the syntactic structure of the method.)
| /** Set ImgProvider object. | ||
| * @param imp */ |
There was a problem hiding this comment.
Incomplete comment. What is the intent of the method? What is the meaning of parameter imp?
| @@ -32,4 +32,13 @@ public void setActive() { | |||
| public void byebye() { | |||
| improvider.setInactive(); | |||
| } | |||
There was a problem hiding this comment.
Add whitespace to separate methods.
| /** @return ImgProvider object. */ | ||
| protected ImgProvider getImgProvider() { | ||
| return improvider; | ||
| } |
There was a problem hiding this comment.
Add whitespace to separate methods.
| * @return current ImageProvider | ||
| */ | ||
|
|
||
| public static ImgProvider getImpro() { |
There was a problem hiding this comment.
Is the name "getImpro" because there is another getImageProvider method that this needs to be distinguished from? (If using a shortened method name, it should still use camel-casing to indicate the beginning of each word.)
| * | ||
| * @param newImpro constructed image provider | ||
| */ | ||
| public static void setImpro(final ImgProvider newImpro) { |
There was a problem hiding this comment.
See comments regarding getImpro method.
|
Closing this PR to submit a chain of PR's that contain fixes to smaller, individual issues by type and/or class. |
|
@cgamble2 Please close this PR so that it no longer shows up as active. |
Fixed the formatting in ImgProvider.java and ImageLab.java to fit appropriate amount of characters per string
Slight formatting fix
Fixed all checkstyle issues on ImgProvider.java
Fixed checkstyle errors for three classes
added setter method to ILFrame class
Correct code convention violations in Run.java
Fixed checkstyle issues with 'impro' variable in ImageLab.java
Removed intellij files
Fixed small issue with image provider variable not being set properly in ImgProvider.java