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

Initial Halstead implementation for Java #811

Merged

Conversation

dburriss
Copy link
Contributor

@dburriss dburriss commented Mar 25, 2022

@dburriss
Copy link
Contributor Author

Test exmaple:

public static void main(String args[]) {
  int a, b, c, avg;
  a = 5; b = 5; c = 5;
  avg = (a + b + c) / 3;
  MessageFormat.format(\"{0}\", avg);
}

I will get to this over the weekend and try to set my test up for expected numbers.
@marco-c who can explain to me the difference between u_operators, operators, u_operands, and operands in the Halstead tests?

@Luni-4
Copy link
Collaborator

Luni-4 commented Mar 25, 2022

Test exmaple:

public static void main(String args[]) {
  int a, b, c, avg;
  a = 5; b = 5; c = 5;
  avg = (a + b + c) / 3;
  MessageFormat.format(\"{0}\", avg);
}

I will get to this over the weekend and try to set my test up for expected numbers. @marco-c who can explain to me the difference between u_operators, operators, u_operands, and operands in the Halstead tests?

@dburriss
u_operators are all distinct operators present in the code
u_operands are all unique operands present in the code
operators are all occurrences of u_operators in the code
operands are all occurrences of u_operands in the code.

You can find what are operators and operands in these tests https://github.com/mozilla/rust-code-analysis/blob/master/src/ops.rs#L312 for different programming languages. First vec are operators and second one operands.

@dburriss
Copy link
Contributor Author

dburriss commented Apr 6, 2022

@Luni-4 I have run into a weird issue.

When parsing the first line public static void main(String args[]){

unknown: program 
unknown: local_variable_declaration 
unknown: modifiers 
unknown: public
unknown: static
operator: void_type
unknown: variable_declarator
operand: identifier
operator: ;
unknown: ERROR
unknown: formal_parameters
operator: (
unknown: formal_parameter
operator: type_identifier
operand: identifier
unknown: dimensions
operator: [
operator: ]
operator: )
unknown: block

Note the operator: ; and unknown: ERROR ... probably related. There is no ; in the first line so this is throwing the count off.

The example is valid Java... I compiled it.

@calixteman
Copy link
Collaborator

It's very likely a matter of adding public class A {and } around your function.

@dburriss
Copy link
Contributor Author

dburriss commented Apr 6, 2022

It's very likely a matter of adding public class A {and } around your function.

Thanks @calixteman
That did the trick. It actually crossed my mind but I had never seen ERROR in the LoC parsing and most of those tests don't have class. Anyway, I learned something. And I am getting the numbers I expect.

@Luni-4 I included a lot of comments and links, etc. in the code explaining what and why I am including different things. I didn't find definitive rules about what is included or excluded.

image

@dburriss dburriss marked this pull request as ready for review April 6, 2022 11:01
@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-halstead branch from 09ad6fa to e05c6d2 Compare April 7, 2022 03:46
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #811 (fa10e44) into master (b5496f8) will decrease coverage by 1.24%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #811      +/-   ##
==========================================
- Coverage   41.24%   40.00%   -1.25%     
==========================================
  Files          52       52              
  Lines        7201     7490     +289     
  Branches     1039     1041       +2     
==========================================
+ Hits         2970     2996      +26     
- Misses       3519     3782     +263     
  Partials      712      712              
Impacted Files Coverage Δ
src/metrics/halstead.rs 72.77% <83.33%> (-0.22%) ⬇️
src/getter.rs 79.83% <100.00%> (+4.32%) ⬆️
src/languages/language_java.rs 3.14% <0.00%> (-86.86%) ⬇️
src/spaces.rs 72.61% <0.00%> (-1.79%) ⬇️
src/metrics/nargs.rs 75.09% <0.00%> (+0.38%) ⬆️
src/node.rs 86.53% <0.00%> (+15.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5496f8...fa10e44. Read the comment docs.

@dburriss
Copy link
Contributor Author

@Luni-4

I looked at ops.rs and added a test but having some problems.

Copying the existing tests quickly and filling in from the list above (or the code), I get the following error:

thread 'ops::tests::java_ops' panicked at 'assertion failed: `(left == right)`
  left: `["()", ")", "+", ",", ".", "/", ";", "=", "[]", "]", "integral_type", "method_invocation", "type_identifier", "void_type", "{}", "}"]`,
 right: `["()", "+", ",", "/", ";", "=", "String", "[]", "format", "int", "void", "{}"]`', src\ops.rs:280:9

A few questions questions:

  1. Do you know why the list for Java is showing the grammar type instead of the value ie. 'integral_typeinstead ofint` as it does for the other languages
  2. Does the order of operators/operands matter in the assert?
  3. Are these println supposed to be here? (hard to believe but they are not mine :) ) https://github.com/mozilla/rust-code-analysis/blob/master/src/ops.rs#L92

@dburriss
Copy link
Contributor Author

@Luni-4 this is failing on the ops.rs file you mentioned. Not something I introduced. The linter has an opinion on something. I can make the change in this PR but I'm not sure how this got in in the first place. New rule maybe?

Log: https://community-tc.services.mozilla.com/tasks/WqxZrlJ2TIKZMbpDibX5Jg/runs/0/logs/public/logs/live_backing.log

@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-halstead branch from fadc2b8 to 2f8ef42 Compare April 14, 2022 15:09
@Luni-4 Luni-4 self-assigned this May 10, 2022
@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-halstead branch from 2f8ef42 to a99ac77 Compare May 26, 2022 10:53
@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-halstead branch from a99ac77 to 1f14f48 Compare May 26, 2022 11:00
@dburriss dburriss mentioned this pull request May 26, 2022
@dburriss
Copy link
Contributor Author

Shuffled things around a bit and moved ops check to #849
Hopefully, it is ok to merge those 2 separately.
@calixteman @Luni-4 ok to review with this scope?

halstead eg

Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Sorry @dburriss for the delay, yeah, it's fine splitting up the two problems.

Just a final request and we can land this PR

src/getter.rs Outdated Show resolved Hide resolved
@dburriss dburriss requested a review from Luni-4 May 29, 2022 06:26
Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Thank you @dburriss! Fine for me!

@Luni-4 Luni-4 merged commit 4987e90 into mozilla:master May 29, 2022
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.

4 participants