Skip to content

Commit

Permalink
Modify rule S5414: move exception code example
Browse files Browse the repository at this point in the history
  • Loading branch information
amelie-renard-sonarsource committed Oct 12, 2023
1 parent db6fa43 commit 9327e7c
Showing 1 changed file with 23 additions and 11 deletions.
34 changes: 23 additions & 11 deletions rules/S5414/cfamily/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
== Why is this an issue?

Mixing (non-const) ``++public++`` and ``++private++`` data members is a bad practice because it causes confusion about the intention of the class:
Mixing (non-const) ``++public++`` and ``++private++`` data members is a bad practice because it confuses the intention of the class:

* If the class is a collection of loosely related values, all the data members should be ``++public++``.
* On the other hand, if the class is trying to maintain an invariant, all the data members should be ``++private++``.
If we mix data members with different levels of accessibility, the purpose of the class is muddled.
If we mix data members with different levels of accessibility, we lose clarity as to the purpose of the class.


=== Noncompliant code example

[source,cpp]
----
class MyClass // Noncompliant
{
class MyClass { // Noncompliant
public:
int firstNumber1() const { return firstNumber; }
void setFirstNumber(int firstNumber) { this->firstNumber = firstNumber; }
int secondNumber = 2;
const int constNumber = 0; // const data members are fine
private:
int firstNumber = 1;
};
Expand All @@ -29,12 +27,10 @@ private:

[source,cpp]
----
class MyClass // Depending on the case, the solution might be different. Here, since this class does not enforce any invariant, we make all the data members public
{
class MyClass { // Depending on the case, the solution might be different. Here, since this class does not enforce any invariant, we make all the data members public
public:
int firstNumber;
int secondNumber;
const int constNumber = 0;
};
----

Expand All @@ -43,11 +39,27 @@ public:

Since ``++const++`` data members cannot be modified, it's not breaking encapsulation to make a const value public, even in a class that enforces an invariant.

[source,cpp]
----
class MyClass { // Compliant by exception
public:
const int constNumber = 0; // const data members are fine
private:
int number = 1;
};
----


== Resources

* https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c134-ensure-all-non-const-data-members-have-the-same-access-level[{cpp} Core Guidelines C.134]: Ensure all non-const data members have the same access level
* https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c9-minimize-exposure-of-members[{cpp} Core Guidelines C.9]: Minimize exposure of members
=== Documentation

* Wikipedia - https://en.wikipedia.org/wiki/Class_invariant[Class invariant]

=== External coding guidelines

* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c134-ensure-all-non-const-data-members-have-the-same-access-level[C.134: Ensure all non-const data members have the same access level]
* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c9-minimize-exposure-of-members[C.9: Minimize exposure of members]


ifdef::env-github,rspecator-view[]
Expand Down Expand Up @@ -79,7 +91,7 @@ Secondaries: one public and one private data member.
Second, is this rule about having in the same class members that are both non-``++const++`` and ``++public++`` along with members that are ``++private++`` with whatever ``++const++``-ness? Because it is not clear to me from the title and description.


In fact, after multiple readings and some cogitation I think this is about having modifiable, ``++public++`` members in a class with ... _anything_ else in it? Because if I can modify the public members directly then what's the point of having methods? They certainly won't be able to notice/act on changes to the public members.
In fact, after multiple readings and some cogitation I think this is about having modifiable, ``++public++`` members in a class with ... _anything_ else in it? Because if I can modify the public members directly then what's the point of having methods? They certainly won't be able to notice/act on changes to the public members.


And finally, I suggest the 2ndary locations highlight every public non-``++const++`` member, not just one of them.
Expand Down

0 comments on commit 9327e7c

Please sign in to comment.