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

Example code and descriptive text are misleading #3886

Closed
jerrywrice opened this issue Apr 14, 2024 · 2 comments · Fixed by #3892
Closed

Example code and descriptive text are misleading #3886

jerrywrice opened this issue Apr 14, 2024 · 2 comments · Fixed by #3892

Comments

@jerrywrice
Copy link

jerrywrice commented Apr 14, 2024

  • I have searched open and closed issues and pull requests for duplicates, using these search terms:
    -Object-Oriented
    -Encapsulation

  • I have checked the latest main branch to see if this has already been fixed, in this file:
    -src/ch17-01-what-is-oo.md

URL to the section(s) of the book with this problem:
https://doc.rust-lang.org/book/ch17-01-what-is-oo.html
Description of the problem:
In Section 17.1 + sub-section 'Encapsulation that Hides Implementation Details', the example with struct 'AveragedCollection' provides the three (3) public trait methods =>

'pub fn add(&mut self, value: i32)'
'pub fn remove(&mut self) -> Option'
'pub fn average(&self) -> f64'

Regarding the supporting text shown next, and specifically the high-lighted sentence =>

Because we’ve encapsulated the implementation details of the struct AveragedCollection, we can easily change aspects, such as the data structure, in the future. For instance, we could use a HashSet instead of a Vec for the list field. As long as the signatures of the add, remove, and average public methods stay the same, code using AveragedCollection wouldn’t need to change. If we made list public instead, this wouldn’t necessarily be the case: HashSet and Vec have different methods for adding and removing items, so the external code would likely have to change if it were modifying list directly.

The (above) descriptive text fails to mention that the specific return value of these trait methods must be identical to the original when invoked in the same order and with the same input argument values. This is essential for the internal (encapsulated) private implementation to be changed and have no measurable impact on existing client code, with the exception of total memory usage, CPU usage, and elapsed run time duration.

Suggested fix:
Replace the existing high-lighted/bold text above with =>

Because we’ve encapsulated the implementation details of the struct AveragedCollection, we can easily change aspects, such as the data structure, in the future. For instance, we could use a HashMap<i32,isize> instead of a Vec for the list field. The signatures of the add, remove, and average public methods must stay the same, and new implementations of the public method functions must be such that client code invoking these public trait methods in a given order and with the same input arguments must return the identical value as did the original. If we had made list public instead, existing client code which directly accessed this field would almost certainly require changes.

@carols10cents
Copy link
Member

The (above) descriptive text fails to mention that the specific return value of these trait methods must be identical to the original when invoked in the same order and with the same input argument values. This is essential for the internal (encapsulated) private implementation to be changed and have no measurable impact on existing client code, with the exception of total memory usage, CPU usage, and elapsed run time duration.

The intent in this section is to say that the code using these methods would not need to change in order to compile, which is all "breaking changes" in the semantic versioning sense means. If there's any change to be made here, I would consider adding the "in order to compile" caveat. Would that solve your issue?

@jerrywrice
Copy link
Author

jerrywrice commented Apr 15, 2024 via email

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 a pull request may close this issue.

2 participants