Skip to content

Commit

Permalink
Scala 2.13: Refactor LiveBlog's findPageWithBlock()
Browse files Browse the repository at this point in the history
`LiveBlogCurrentPage.findPageWithBlock()` is responsible for making sure
that when a user clicks on a live-blog permalink to a **block**, eg:

https://www.theguardian.com/politics/live/2022/aug/01/tory-leadership-race-rishi-sunak-lizz-truss-vote-keir-starmer-uk-politics-live?page=with:block-62e7e89d8f08730a1f7f5579#block-62e7e89d8f08730a1f7f5579

...the user is taken to the correct 'page' of the live blog - page 3 of 5
in the example above, for instance. The code was was first introduced in
January 2016 with #11700,
subsequently updated with #13566,
etc.

The existing code compiles without warnings under Scala 2.12, but the
Scala 2.13 compiler gives a warning that not _all_ conceivable cases
in the pattern-match are handled:

```
frontend/common/app/model/LiveBlogCurrentPage.scala:190:12: match may not be exhaustive.
[warn] It would fail on the following inputs: List(_), Nil
[warn]       .map {
[warn]            ^
```

In this particular case, the warning's unnecessary - the preceding `.sliding(3)`:

https://github.com/guardian/frontend/blob/f90c8a58e6f0941c96392d14fe27e61a4dbaf25b/common/app/model/LiveBlogCurrentPage.scala#L188

...means that the `List` supplied to the case match expression will
_always_ have 3 elements, and that's precisely the case that is handled -
but there's no way for the compiler to know that - so we as devs need to
do _something_ to make the warning go away!

Some options:

* Use the @unchecked annotation (see https://www.scala-lang.org/api/2.12.7/scala/unchecked.html)
  This is an option of last resort - turning off compiler checks leaves us
  exposed to runtime errors and is generally a bad idea!
* Use `collect()` rather than `map()` - this accepts a partial function,
  rather than a total one, so the compiler error will go away.
  https://www.scala-lang.org/api/2.12.7/scala/collection/immutable/Seq.html#collectFirst[B](pf:PartialFunction[A,B]):Option[B]
* Refactor so that the code no longer needs to assume that a `List` type
  (which can have any length) has length `3`.

In the end I decided to go with the refactor, because there were a few
things about the existing code that could be tweaked:

* Unnecessary work: The method was creating a `LiveBlogCurrentPage` for
  _every_ page in the liveblog, even though it only ever needed one
  (the single page that the block actually exists on!), and would
  eventually throw away the rest.
* The logic around padding the front & end of the `pages` List with
  two `None` entries, to allow extracting the `newer` & `older` pages,
  totally worked but added an extra step to the code (and therefore a
  bit of complexity for humans to understand: "what are endedPages?")
  and could be replaced by just incrementing/decrementing the page index
  for the one `LiveBlogCurrentPage` we're now creating.
* Various variables could be inlined to the point of creation on the
  `LiveBlogCurrentPage` case class. By using named parameters in constructing
  the case class, no clarity is lost, and the code is more concise.

See also:

* The Scala 2.13 upgrade PR: #25190
  • Loading branch information
rtyley committed Aug 3, 2022
1 parent f90c8a5 commit f86dba2
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 41 deletions.
2 changes: 1 addition & 1 deletion article/test/model/LiveBlogCurrentPageTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ class LiveBlogCurrentPageTest extends AnyFlatSpec with Matchers {
val result = LiveBlogCurrentPage.findPageWithBlock(
pageSize = 2,
blocks = testFakeBlocks.blocksSequence,
isRequestedBlock = "3",
requestedBlockId = "3",
filterKeyEvents = false,
topicResult = Some(topicResult),
)
Expand Down
78 changes: 38 additions & 40 deletions common/app/model/LiveBlogCurrentPage.scala
Original file line number Diff line number Diff line change
Expand Up @@ -162,52 +162,50 @@ object LiveBlogCurrentPage {
def findPageWithBlock(
pageSize: Int,
blocks: Seq[BodyBlock],
isRequestedBlock: String,
requestedBlockId: String,
filterKeyEvents: Boolean,
topicResult: Option[TopicResult],
): Option[LiveBlogCurrentPage] = {
val pinnedBlock = blocks.find(_.attributes.pinned).map(renamePinnedBlock)
val filteredBlocks = applyFilters(blocks, filterKeyEvents, topicResult)
val (mainPageBlocks, restPagesBlocks) = getPages(pageSize, filteredBlocks)
val newestPage = FirstPage(mainPageBlocks, filterKeyEvents, topicResult)
val pages = newestPage :: restPagesBlocks.zipWithIndex
.map {
case (page, index) =>
// page number is index + 2 to account for first page and 0 based index
BlockPage(blocks = page, blockId = page.head.id, pageNumber = index + 2, filterKeyEvents, topicResult)
val pages: Seq[PageReference] = pagesFromBodyBlocks(pageSize, blocks, filterKeyEvents, topicResult)

val indexOfPageWithRequestedBlock = pages.indexWhere(_.blocks.exists(_.id == requestedBlockId))
if (indexOfPageWithRequestedBlock < 0) None
else
Some {
val currentPage = pages(indexOfPageWithRequestedBlock)
val isNewestPage = pages.head == currentPage

LiveBlogCurrentPage(
currentPage = currentPage,
pagination =
if (pages.size <= 1) None
else
Some(
N1Pagination(
newest = if (isNewestPage) None else Some(pages.head),
newer = pages.lift(indexOfPageWithRequestedBlock - 1),
oldest = if (pages.last == currentPage) None else Some(pages.last),
older = pages.lift(indexOfPageWithRequestedBlock + 1),
numberOfPages = pages.size,
),
),
pinnedBlock = if (isNewestPage) blocks.find(_.attributes.pinned).map(renamePinnedBlock) else None,
)
}
val oldestPage = pages.lastOption.getOrElse(newestPage)

val endedPages = None :: (pages.map(Some.apply) :+ None)
}

def hasRequestedBlock(page: LiveBlogCurrentPage): Boolean = {
page.currentPage.blocks.exists(_.id == isRequestedBlock)
private def pagesFromBodyBlocks(
pageSize: Int,
blocks: Seq[BodyBlock],
filterKeyEvents: Boolean,
topicResult: Option[TopicResult],
): Seq[PageReference] = {
val (mainPageBlocks, restPagesBlocks) = getPages(pageSize, applyFilters(blocks, filterKeyEvents, topicResult))
FirstPage(mainPageBlocks, filterKeyEvents, topicResult) :: restPagesBlocks.zipWithIndex.map {
case (page, index) =>
// page number is index + 2 to account for first page and 0-based index of `zipWithIndex`
BlockPage(blocks = page, blockId = page.head.id, pageNumber = index + 2, filterKeyEvents, topicResult)
}

endedPages
.sliding(3)
.toList
.map {
case List(newerPage, Some(currentPage), olderPage) =>
val isNewestPage = newestPage.equals(currentPage)
LiveBlogCurrentPage(
currentPage = currentPage,
pagination =
if (pages.length > 1)
Some(
N1Pagination(
newest = if (isNewestPage) None else Some(newestPage),
newer = newerPage,
oldest = if (oldestPage.equals(currentPage)) None else Some(oldestPage),
older = olderPage,
numberOfPages = pages.length,
),
)
else None,
if (isNewestPage) pinnedBlock else None,
)
}
.find(hasRequestedBlock)
}

private def renamePinnedBlock(pinnedBlock: BodyBlock): BodyBlock = {
Expand Down

0 comments on commit f86dba2

Please sign in to comment.