codereview.md

0. Sample code

// FooCrawler
fun readPage(url: String): Pair<String, String?> {
    try {
        val content = fetchPage(url)
        ...
        val nextPageUrl = extractNextPageUrl(content)
        return Pair(content, nextPageUrl)
    } catch (e: Exception) {
        return Pair("", null)
    }
}

1.

// reference code:
fun readPage(url: String): Pair<String, String?> {

The return value is not clear. It's hard to understand what the first and second elements of the pair represent.

To improve, we can choose some of the following options:

1. Define a data model to represent the result.

data class PageResult(val content: String, val nextPageUrl: String?)

2. Rename the function to clarify its purpose, eg. fetchPageContentAndNextUrl.
3. Add documentation to explain the return values.

/**
 * Reads the content of a page and extracts the next page URL.
 *
 * @return A pair containing the page content and the next page URL, 
 * or an empty string and null if an error occurs.
 */

Besides, we should also mention when the second element can be null. In this case, empty is used when no more pages are available, and null is used when an error occurs. Having a documentation comment helps clarify this behavior.

1.1.

Btw, if we want to let the caller know the error details, we can create a sealed class to separate the success and error cases:

sealed class PageResult {
    data class Success(
        val content: String, 
        val nextPageUrl: String
    ) : PageResult()
    data class Error(val errorType: ErrorType) : PageResult()
}

This way, we can use non-nullable for nextPageUrl for the success case.

2.

// reference code:
try {
    val content = fetchPage(url)
    ...
    val nextPageUrl = extractNextPageUrl(content)
    return Pair(content, nextPageUrl)
}

The try block is too broad. In this case, only fetchPage can throw an exception, so we should only wrap that call in a try-catch block. This way, we can handle specific exceptions related to fetching the page without catching unrelated exceptions that might occur in the subsequent code.

val content = try {
    fetchPage(url)
} catch (e: FetchPageException) {
    null
} ?: return Pair("", null)
...
val nextPageUrl = extractNextPageUrl(content)
return Pair(content, nextPageUrl)

3.

// reference code:
} catch (e: Exception) {

Catching a general Exception is not a good practice. It can hide specific exceptions that we might want to handle differently. Instead, we should catch specific exceptions that we expect to occur, In this case, only FetchPageException is relevant, so we can catch that specifically.

} catch (e: FetchPageException) {
    return Pair("", null)
}