Skip to content

fix: Books without readings being spuriously included in statistics.#674

Merged
mateusz-bak merged 1 commit intomateusz-bak:masterfrom
rmarker:statsLength
Jul 20, 2025
Merged

fix: Books without readings being spuriously included in statistics.#674
mateusz-bak merged 1 commit intomateusz-bak:masterfrom
rmarker:statsLength

Conversation

@rmarker
Copy link
Contributor

@rmarker rmarker commented May 24, 2025

It was the case that books without recorded readings would be included in some of the statistics for a specific year. Specifically the shortest/longest, and average pages statistics.
It now will only include these books in overall statistics where a specific year isn't provided.

It was the case that books without recorded readings would be included
in some of the statistics for a specific year. Specifically the
shortest/longest, and average pages statistics. It now will only include
these books in overall statistics where a specific year isn't provided.
Copy link

@RomanKovtyukh RomanKovtyukh left a comment

Choose a reason for hiding this comment

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

Review

This change looks good to me.

Further improvement

The file needs some refactoring. We have the same logic copy-pasted multiple times in the file and it's not only for the changes from this PR.

For example having some List<Book> filterBooksByYears(List<int> years) and List<Reading> filterReadingsByYears(List<int> years) would be useful in multiple places in this file.

@RomanKovtyukh
Copy link

The workflow failures are not related to the PR. Separate PR to fix the workflow: #679

@mateusz-bak
Copy link
Owner

Fixes the issue, thanks!

@mateusz-bak mateusz-bak merged commit 53e58ad into mateusz-bak:master Jul 20, 2025
0 of 4 checks passed
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.

3 participants