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

Added calculation for relational cohesion metric #747

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

kmkristof
Copy link
Collaborator

@kmkristof kmkristof commented May 8, 2024

Closes #680

Added -m flag to the parser which can be used to specify modules. The user is expected to input a path to a file in which the paths to the modules are listed. If no modules are specified the default behaviour is to treat every directory under the input paths as a module.

  • Add test cases

@intjftw intjftw added Kind: Enhancement 🌟 Status: WIP 👷 Issue or PR under development - feel free to review, though! Plugin: C++ Issues related to the parsing and presentation of C++ projects. Plugin: Metrics Issues related to the code metrics plugin. labels May 8, 2024
@kmkristof kmkristof marked this pull request as ready for review June 25, 2024 07:50
@mcserep mcserep requested review from mcserep and intjftw July 1, 2024 13:46
@mcserep mcserep added this to the Upcoming Release milestone Jul 1, 2024
@mcserep
Copy link
Collaborator

mcserep commented Jul 1, 2024

@kmkristof The PR was marked ready, but the description says the tests are still missing. (And there are indeed no tests in the changeset.) Please add the missing unit tests.

@mcserep
Copy link
Collaborator

mcserep commented Jul 5, 2024

@kmkristof The PR contains various commits unrelated to this PR for some reason. This makes it hard to review the PR.
Please rebase your branch onto master and force push it.

@@ -18,6 +20,7 @@
#include <util/odbtransaction.h>

#include <memory>
#include <iostream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this required? fstream should be enough for file management.

object(CppAstNode : CppRecord::astNodeId == CppAstNode::id) \
object(File : CppAstNode::location.file) \
object(CppMemberType : CppMemberType::memberAstNode)
struct RelationalCohesionRecordView
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the same view is used in #757. In this case a more generic name should be used and code duplication should be avoided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A common view could probably be used however the same applies to "CohesionCppRecordView" for example ("RelationalCohesionRecordView" contains this + typeHash).

Maybe this should be looked at separately to unify the commonly used views?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You started working concurrently on a second issue before finishing this one. I don't advise to merge in duplicated code segments so close to each other in time, and then refactor them in a follow-up issue.


void CppMetricsParser::relationalCohesion()
{
std::unordered_set<std::string> filepaths;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be filePaths, following naming guidelines.

plugins/cpp_metrics/parser/src/cppmetricsparser.cpp Outdated Show resolved Hide resolved
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp Outdated Show resolved Hide resolved
typeDefinitionPaths.insert(std::make_pair(record.typeHash,record.filePath)); //save where the type is defined to avoid self relation
}

std::unordered_map<std::string,std::unordered_set<std::uint64_t>> relationsFoundInFile; //store the type relations already found for each file
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what unordered_multimap is for. Wouldn't that simplify the code?

plugins/cpp_metrics/parser/src/cppmetricsparser.cpp Outdated Show resolved Hide resolved
plugins/cpp_metrics/parser/src/cppmetricsparser.cpp Outdated Show resolved Hide resolved
@kmkristof
Copy link
Collaborator Author

@mcserep I added the necessary data and definitions for unit testing however i ran into a problem.
For example when looking at plugins/cpp_metrics/test/sources/parser/RelationalCohesion/MultipleTypesInModuleWithRelations/RelationByFunctionParameter/relatedByFPA no typeHash is generated for the class itself (as it doesn't become a CppTypedEntity).
This means that when i find a reference to this type i cannot connect it to the definition. How do you suggest i solve this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind: Enhancement 🌟 Plugin: C++ Issues related to the parsing and presentation of C++ projects. Plugin: Metrics Issues related to the code metrics plugin. Status: WIP 👷 Issue or PR under development - feel free to review, though!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relational Cohesion at Module Level for C++
4 participants