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

Fixing squid: S1192 String literals should not be duplicated #105

Closed
wants to merge 1 commit into from
Closed

Fixing squid: S1192 String literals should not be duplicated #105

wants to merge 1 commit into from

Conversation

devFozgul
Copy link
Contributor

@devFozgul devFozgul commented Jun 8, 2016

This pull request is focused on resolving occurrences of Sonar rule
squid:S1192 - “String literals should not be duplicated”.
This PR will reduce 120 min TD.
You can find more information about the issue here:
https://dev.eclipse.org/sonar/rules/show/squid:S1192
Please let me know if you have any questions.
Fevzi Ozgul

@gallandarakhneorg gallandarakhneorg added this to the 13.0 milestone Jun 8, 2016
@gallandarakhneorg gallandarakhneorg self-assigned this Jun 8, 2016
@gallandarakhneorg
Copy link
Owner

Please use the i18n standard for coding the string literals. See the rest of the AFC code for examples.

@devFozgul
Copy link
Contributor Author

I wonder why you closed this. I could modify it according to the i18n
standard after seeing the examples.

On Wed, Jun 8, 2016 at 12:33 PM, Stéphane Galland notifications@github.com
wrote:

Closed #105 #105.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AR09BIDeM6-i3qf3u7OOMoUfpd4PJzHDks5qJox5gaJpZM4Iwvao
.

@devFozgul
Copy link
Contributor Author

Could you please give me a few good examples of the files in which I can
find examples of the standard? Thank you.

On Wed, Jun 8, 2016 at 12:51 PM, Stéphane Galland notifications@github.com
wrote:

Reopened #105 #105.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AR09BNv_nvFdjCjZ-hIbGNrO-2CghQzzks5qJpC-gaJpZM4Iwvao
.

@gallandarakhneorg
Copy link
Owner

Example: https://github.com/gallandarakhneorg/afc/blob/master/core/math/src/main/java/org/arakhne/afc/math/geometry/d2/afp/Path2afp.java#L109

The string E1 is a valid in-code string (note the NON-NLS comment at the end of the line) that is the ID of the message in the Path2afp.properties file.

Please configure your Sonar for avoiding to have squid:S1192 when NON-NLS comment is present.

@devFozgul
Copy link
Contributor Author

Ok. I would like to ask before I prepare a PR on an issue, namely squid:
S2975 '"clone" should not be used'; meaning a copy constructor should be
used instead... There are quite a few places in the project with cloning
used. Would you be interested in my converting them to copy constructor?

On Wed, Jun 8, 2016 at 1:15 PM, Stéphane Galland notifications@github.com
wrote:

Example:
https://github.com/gallandarakhneorg/afc/blob/master/core/math/src/main/java/org/arakhne/afc/math/geometry/d2/afp/Path2afp.java#L109

The string E1 is a valid in-code string (note the NON-NLS comment at the
end of the line) that is the ID of the message in the Path2afp.properties
file.

Please configure your Sonar for avoiding to have squid:S1192 when NON-NLS
comment is present.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BLrP9G7IJP7sr-UpuAu0wo54YOCKks5qJpYzgaJpZM4Iwvao
.

@gallandarakhneorg
Copy link
Owner

Don't remove clones.
But you could add copy-constructors. Don't forget to do a deep copy in the copy-constructors (that is the meaning of the clones).
We are providing the two features since the AFC library may be used with one or the other approach by the AFC users. Additionally, clone may be faster that copy-constructors since it is a "low-level" copy of the memory.

@devFozgul
Copy link
Contributor Author

OK.

On Wed, Jun 8, 2016 at 1:36 PM, Stéphane Galland notifications@github.com
wrote:

Don't remove clones.
But you could add copy-constructors. Don't forget to do a deep copy in the
copy-constructors (that is the meaning of the clones).
We are providing the two features since the AFC library may be used with
one or the other approach by the AFC users. Additionally, clone may be
faster that copy-constructors since it is a "low-level" copy of the memory.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BHAMpSgLP-zDSbR4QHz39RbbR-3pks5qJpsxgaJpZM4Iwvao
.

@devFozgul
Copy link
Contributor Author

Moving to another rule squid: S1168 "empty arrays and collections should
be returned instead of null". May I convert the null returns to empty
arrays and collections?

On Wed, Jun 8, 2016 at 1:41 PM, Fevzi Ozgul fevzi.ozgul@devfactory.com
wrote:

OK.

On Wed, Jun 8, 2016 at 1:36 PM, Stéphane Galland <notifications@github.com

wrote:

Don't remove clones.
But you could add copy-constructors. Don't forget to do a deep copy in
the copy-constructors (that is the meaning of the clones).
We are providing the two features since the AFC library may be used with
one or the other approach by the AFC users. Additionally, clone may be
faster that copy-constructors since it is a "low-level" copy of the memory.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BHAMpSgLP-zDSbR4QHz39RbbR-3pks5qJpsxgaJpZM4Iwvao
.

@gallandarakhneorg
Copy link
Owner

I'm almost agree with S1168. If you do, don't create new instances of collections but use the Collections utilities for creating the empty array/collections.

@devFozgul
Copy link
Contributor Author

Ok. That is What I intended doing... Using the Collections library
available. If there are tests AssertNull for those outputs, also those
tests will be modified accordingly. Because we are not returning null
anymore but an object but empty object.

On Wed, Jun 8, 2016 at 2:16 PM, Stéphane Galland notifications@github.com
wrote:

I'm almost agree with S1168. If you do, don't create new instances of
collections but use the Collections utilities for creating the empty
array/collections.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BClE55zAbaAb-7eWsRBrwIrAGu0hks5qJqSUgaJpZM4Iwvao
.

@devFozgul
Copy link
Contributor Author

I have just cloned a fresh version of the project. But I have 6 compiler
errors like "Error:(62, 39) java: type argument ? extends V is not within
bounds of type-variable RV" resulting from
core\math\src\main\java\org\arakhne\afc\math\extensions\xtext\Tuple2DExtensions.java
class. Do you know what might be the reason?

On Wed, Jun 8, 2016 at 2:19 PM, Fevzi Ozgul fevzi.ozgul@devfactory.com
wrote:

Ok. That is What I intended doing... Using the Collections library
available. If there are tests AssertNull for those outputs, also those
tests will be modified accordingly. Because we are not returning null
anymore but an object but empty object.

On Wed, Jun 8, 2016 at 2:16 PM, Stéphane Galland <notifications@github.com

wrote:

I'm almost agree with S1168. If you do, don't create new instances of
collections but use the Collections utilities for creating the empty
array/collections.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BClE55zAbaAb-7eWsRBrwIrAGu0hks5qJqSUgaJpZM4Iwvao
.

@gallandarakhneorg
Copy link
Owner

Are you using Maven for compiling? Are you using JDT/ECJ?

@devFozgul
Copy link
Contributor Author

I am using maven. What are the others. Previously I was not getting these
errors.

On Wed, Jun 8, 2016 at 4:20 PM, Stéphane Galland notifications@github.com
wrote:

Are you using Maven for compiling? Are you using JDT/ECJ?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BEstRaEiY3gN5f-i0hcUFrw8DF8Aks5qJsGKgaJpZM4Iwvao
.

@gallandarakhneorg
Copy link
Owner

gallandarakhneorg commented Jun 8, 2016

JDT/ECJ is the compiler used by IBM/Eclipse. We have change the default compiler used by Maven for moving from javac to ecj. Indeed, ecj is much more better than javac for resolving generic types. If you are using an IDE that is not Eclipse, please ensure that is it using the JDT/ECJ, not javac.

See #86 for following the issue related to JDT/ECJ.

@gallandarakhneorg
Copy link
Owner

Ok. I have also some issues on my side from the fresh clone. I will fix them asap.

@devFozgul
Copy link
Contributor Author

I am now switching to Eclipse IDE. I am setting the project on my laptop
as I will be using it while I am away from home for next week. Eclipse is
available on my laptop. In IntelliJ Idea I could easily incorporate
checkstyle.xml via Code Style settings. Is it the same with Eclipse. I have
never used Eclipse's checkstyle setting.
In Eclipse I am used to importing projects as Maven. So for JDT/ECJ, How do
I set Eclipse IDE?

On Wed, Jun 8, 2016 at 4:32 PM, Stéphane Galland notifications@github.com
wrote:

JDT/ECJ is the compiler used by IBM/Eclipse. We have change the default
compiler used by Maven for moving from javac to ecj. Indeed, ecj is much
more better than javac for resolving generic types. If you are using an IDE
that is not Eclipse, please ensure that is it using the JDT/ECJ, not javac.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BC-J0i2gygV2O5ghRTIe82mnfJJFks5qJsRSgaJpZM4Iwvao
.

@gallandarakhneorg
Copy link
Owner

In Eclipse, after import the AFC source code as Maven project, you may have a couple of errors. these errors could be easily solved by installing an Eclipse adapter (by clicking on the automatic problem solving option). Checkstyle has an Eclipse plugin that could be automatically installed in this way. You need to have Internet connexion.

@gallandarakhneorg
Copy link
Owner

I have pushed a code that seems to compile successfully on command line. Compilation results by our CI server are available on Travis.

@devFozgul
Copy link
Contributor Author

Ok. I will try again.

On Wed, Jun 8, 2016 at 4:48 PM, Stéphane Galland notifications@github.com
wrote:

I have pushed a code that seems to compile successfully on command line.
Compilation results by our CI server are available on Travis
https://travis-ci.org/gallandarakhneorg/afc/builds/136152553.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#105 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AR09BIIUjhO5Lp9e-mdhit0BFdyXM57cks5qJsg0gaJpZM4Iwvao
.

@gallandarakhneorg gallandarakhneorg modified the milestones: 14.0, 13.0 Jun 11, 2016
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.

2 participants