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

(#120) TCC: advancing implementation #182

Merged
merged 5 commits into from
Feb 17, 2018
Merged

(#120) TCC: advancing implementation #182

merged 5 commits into from
Feb 17, 2018

Conversation

llorllale
Copy link
Contributor

@llorllale llorllale commented Feb 14, 2018

as per #120

TCC(C) = NDC(C) / NP(C), where C is the class, NP(C) is a
maximal possible number of direct or indirect connections - N * (N - 1) / 2,
NDC(C) is a number of direct connections. Value of the metric is in range [0, 1],
greater is better.

See the bieman95.pdf paper for more details.

@0crat 0crat added the scope label Feb 14, 2018
@0crat
Copy link
Collaborator

0crat commented Feb 14, 2018

Job #182 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Feb 14, 2018

This pull request #182 is assigned to @amihaiemil/z, here is why. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @yegor256/z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

@llorllale
Copy link
Contributor Author

llorllale commented Feb 14, 2018

@amihaiemil weird... let me check the test failure in the build

@llorllale
Copy link
Contributor Author

@amihaiemil please review

@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #182 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #182   +/-   ##
=========================================
  Coverage     73.93%   73.93%           
  Complexity      159      159           
=========================================
  Files            31       31           
  Lines          1013     1013           
  Branches         71       71           
=========================================
  Hits            749      749           
  Misses          232      232           
  Partials         32       32

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 c4c7bfd...9b1bfa8. Read the comment docs.

@llorllale
Copy link
Contributor Author

@amihaiemil I'm actually a bit confused by this puzzle. It said that we needed to take into account when two methods are related because they both call a third one. So I implemented that.

But through a cursory look at the PDF bieman95.pdf I found this:

The direct connectivity between methods is determined
from the class abstraction. If there exists one or more
common instance variables between two method abstractions then the two corresponding methods are directly
connected.

So it seems like TCC was already implemented? WDYT?

@llorllale
Copy link
Contributor Author

@amihaiemil I've read the paper and produced some notes. I'll be making some changes and let you know. Thank you for your patience

@llorllale
Copy link
Contributor Author

@amihaiemil please review

@llorllale llorllale changed the title (#120) TCC: establishing relations between methods when both call a 3rd method of the class (#120) TCC Feb 15, 2018
@amihaiemil
Copy link
Contributor

@llorllale

I'm actually a bit confused by this puzzle

Sorry, but I can't help you with details related to the metric itself. I don't know them. Here, I am paying more attention to the style, size, Java code in the PR etc and the puzzles.

Basically, since these are sandbox projects, the goal is for the new guys to learn how the model works. If there's a bug in a metric (which is highly expected, since nobody is an expert in theoretical software metrics here), it's not the end of the world. Plus, we have the ARCH, who is supposed to look at the things with more substance :)

@amihaiemil
Copy link
Contributor

amihaiemil commented Feb 16, 2018

@llorllale From my point of view, it looks good. Maybe, after the new puzzle is created by 0pdd, go there and link the notes that you've made here

@amihaiemil
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 16, 2018

@rultor merge

@amihaiemil Thanks for your request. @yegor256 Please confirm this.

@llorllale
Copy link
Contributor Author

@amihaiemil my confusion stemmed from blindly following through with the puzzle's instructions. After reading the PDF for myself I realized that the instructions did not apply to TCC. So I wasted quite a lot of time on this one :)

@llorllale
Copy link
Contributor Author

@yegor256 please merge

@yegor256
Copy link
Member

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 17, 2018

@rultor merge

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

@rultor rultor merged commit 9b1bfa8 into cqfn:master Feb 17, 2018
@rultor
Copy link
Collaborator

rultor commented Feb 17, 2018

@rultor merge

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

@0crat
Copy link
Collaborator

0crat commented Feb 17, 2018

@ypshenychka/z please review this job, as in §30

@llorllale llorllale deleted the 120 branch February 17, 2018 16:42
@amihaiemil
Copy link
Contributor

@ypshenychka Can you give the verdict here?

@ypshenychka
Copy link

@amihaiemil Don't worry. I will review as soon as the issue https://github.com/zerocracy/farm/issues/592 is resolved.

@0crat
Copy link
Collaborator

0crat commented Feb 19, 2018

@amihaiemil/z this job was assigned to you 5 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

@ypshenychka
Copy link

@llorllale According to our QA Rules:

Pull request title explains the problem it is fixing.
Pull request description explains the solution proposed and contains a link to the original ticket it is related to.

Please provide more informative title and description.

@llorllale
Copy link
Contributor Author

@ypshenychka would the following suffice?

TCC(C) = NDC(C) / NP(C), where C is the class, NP(C) is a
maximal possible number of direct or indirect connections - N * (N - 1) / 2,
NDC(C) is a number of direct connections. Value of the metric is in range [0, 1],
greater is better.
See the bieman95.pdf paper for more details.

@ypshenychka
Copy link

@llorllale

llorllale commented 5 days ago
weird... let me check the test failure in the build

According to our QA Rules:

Messages in a job always start with a name of a user they are addressed to.

Please correct your message by indicating an addressee in the beginning.

@ypshenychka
Copy link

@llorllale Sure. Please go ahead and edit title\description.

@llorllale llorllale changed the title (#120) TCC (#120) TCC: advancing implementation Feb 19, 2018
@llorllale
Copy link
Contributor Author

@ypshenychka fixed title, description, and that unaddressed message you pointed out

@ypshenychka
Copy link

@amihaiemil According to our QA Rules:

The code reviewer found at least three problems in the code.
Comments were mostly about design problems, not cosmetic issues.

No issues were found during code review. Please confirm that you'll try to find at least three problems while future reviews.

@ypshenychka
Copy link

@llorllale Great. Thanks.

@amihaiemil
Copy link
Contributor

@ypshenychka right, I'll try to find more bugs in future tasks. But since these are sandbox projects, I am paying more attention to the methodology :)

@ypshenychka
Copy link

@amihaiemil thank you

@ypshenychka
Copy link

@0crat quality acceptable

@0crat
Copy link
Collaborator

0crat commented Feb 19, 2018

QA review completed: +15 points just awarded to @ypshenychka/z, total is +240

@0crat
Copy link
Collaborator

0crat commented Feb 19, 2018

Order was finished, quality was "acceptable": +15 points just awarded to @amihaiemil/z, total is +730

@0crat
Copy link
Collaborator

0crat commented Feb 19, 2018

Payment to ARC for a closed pull request, as in §28: +15 points just awarded to @yegor256/z, total is +10274

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants