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

#19 OCC #21

Merged
merged 4 commits into from
Oct 26, 2017
Merged

#19 OCC #21

merged 4 commits into from
Oct 26, 2017

Conversation

vssekorin
Copy link
Contributor

As per #19
OCC.java introduced

Now the class contains a very dirty code. I would like to receive advice on correcting.

@0crat
Copy link
Collaborator

0crat commented Oct 24, 2017

@yegor256 please, pay attention to this pull request

@0crat
Copy link
Collaborator

0crat commented Oct 24, 2017

Job gh:yegor256/jpeek#21 is in scope.

@codecov-io
Copy link

codecov-io commented Oct 24, 2017

Codecov Report

Merging #21 into master will increase coverage by 0.2%.
The diff coverage is 96.62%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master      #21     +/-   ##
===========================================
+ Coverage     94.11%   94.32%   +0.2%     
- Complexity       53       93     +40     
===========================================
  Files             8       10      +2     
  Lines           187      317    +130     
  Branches         21       41     +20     
===========================================
+ Hits            176      299    +123     
- Misses            5       10      +5     
- Partials          6        8      +2
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/jpeek/metrics/cohesion/OCC.java 96.62% <96.62%> (ø) 33 <33> (?)
src/main/java/org/jpeek/App.java 89.18% <0%> (-10.82%) 5% <0%> (ø)
.../main/java/org/jpeek/metrics/JavassistClasses.java 100% <0%> (ø) 13% <0%> (+3%) ⬆️
src/main/java/org/jpeek/metrics/cohesion/CAMC.java 92.3% <0%> (ø) 15% <0%> (ø) ⬇️
src/main/java/org/jpeek/Report.java 100% <0%> (ø) 4% <0%> (?)

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 97753b2...4b3ddaf. Read the comment docs.

import org.xembly.Directive;

/**
* Optimistic Class Cohesion (OCC).
Copy link
Member

Choose a reason for hiding this comment

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

@vssekorin please, add a brief explanation of the logic here, like it's done in LCOM.java

@yegor256
Copy link
Member

@vssekorin indeed, it looks complex, but not messy. You can move that inner class out of the method, but it seems that such a refactoring will only decrease readability. I would leave everything the way it is now and only fix the things I mentioned above.

@yegor256
Copy link
Member

@vssekorin good code, by the way, thanks!

* @author Vseslav Sekorin (vssekorin@gmail.com)
* @version $Id$
* @see <a href="https://www.researchgate.net/publication/268046583_A_Proposal_of_Class_Cohesion_Metrics_Using_Sizes_of_Cohesive_Parts">A Proposal of Class Cohesion Metrics Using Sizes of Cohesive Parts</a>
* @since 0.2
Copy link
Member

Choose a reason for hiding this comment

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

@vssekorin should be 0.4

XhtmlMatchers.xhtml(
new Xembler(
new OCC(
new FakeBase("Test")
Copy link
Member

Choose a reason for hiding this comment

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

@vssekorin can you give this class a more meaningful name? You're not using Foo for some reason. What is that reason? The name should say it. Make it TooComplex or TwoAttributes, something like that, to explain us what these classes are about.

XhtmlMatchers.xhtml(
new Xembler(
new OCC(
new FakeBase("C")
Copy link
Member

Choose a reason for hiding this comment

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

@vssekorin the same here, C is not really a good name :)

@yegor256
Copy link
Member

@vssekorin one more thing, don't forget to update the README and explain the metric there.

@yegor256
Copy link
Member

yegor256 commented Oct 25, 2017

@vssekorin and please add PDF to this folder: https://github.com/yegor256/jpeek/tree/master/papers

@vssekorin
Copy link
Contributor Author

@yegor256 thanks for your review.

@yegor256
Copy link
Member

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Oct 26, 2017

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 4b3ddaf into cqfn:master Oct 26, 2017
@rultor
Copy link
Collaborator

rultor commented Oct 26, 2017

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min)

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.

5 participants