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 simplifications in AbstractMojo #47

Merged
merged 4 commits into from
Aug 14, 2022
Merged

Conversation

rhowe
Copy link
Contributor

@rhowe rhowe commented Aug 10, 2022

Clean up a few overly-verbose code constructs:

  • Replace an unnecessary use of StringBuilder with string concatenation.
  • Remove some code duplication when selecting which archiver to use.
  • Eliminate an else branch by returning early in AbstractMojo#projectHasAlreadySetAnArtifact()
  • Remove an unnecessary local variable in AbstractMojo#hasClassifier()

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

lgtm
just minor comment.
maybe it's only me but I find adding final not adding any real value here

@@ -196,16 +196,17 @@ protected File getJarFile( File basedir, String resultFinalName, String classifi
throw new IllegalArgumentException( "finalName is not allowed to be null" );
}

StringBuilder fileName = new StringBuilder( resultFinalName );

final String fileName;
Copy link
Member

Choose a reason for hiding this comment

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

not sure what is the need if final here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No real need, it's mostly just syntactic sugar in this case. It's somewhat a statement of intent indicating that the value will be calculated only once.
This was the kind of feedback I was seeking though - if you're happy with the changeset in principle, I'll take the finals out, cut a Jira ticket and make the PR more presentable etc.

Copy link
Member

Choose a reason for hiding this comment

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

all good. please remove those final as I don't think they make sense for a local variable. except adding more useless code.
btw I'm more a sugar free person if it's not really needed 🤣
if you want to create a jira why not but I'm not sure we need a Jira for such code polishing.
it's up2u

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The less I need to touch Jira the better. OK, I've removed the finals and updated the PR description

Copy link
Member

Choose a reason for hiding this comment

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

@rhowe great. Thanks! Still on final to remove ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, OK the final final is gone

@@ -243,18 +244,11 @@ public File createArchive()
}
}

final String archiverName = containsModuleDescriptor ? "mjar" : "jar";
Copy link
Member

Choose a reason for hiding this comment

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

why final? no real need of this

Copy link
Member

Choose a reason for hiding this comment

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

oh oh, I know people they insinst on final...

@rhowe rhowe changed the title Code simplifications Code simplifications in AbstractMojo Aug 11, 2022
Inverting this test leads to a more readable flow.
We can remove all branching from this method and return the check
directly.
@olamy olamy merged commit 4148491 into apache:master Aug 14, 2022
@rhowe rhowe deleted the simpler-code branch August 14, 2022 17:02
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