Рефакторинг функций

#инспекция_кода #f#

Нужно вытащить информацию с некоторого сайта. Для разбора HTML использую F# Data:
HTML Parser (HTML Type Provider, к сожалению, в данном случае неприменим). 

Реализовал следующим образом:

let getNextLink (document : HtmlDocument) = 
    document.Descendants "a"
    |> Seq.choose
        (fun node ->
            match node.TryGetAttribute "href" with
            |Some href when node.InnerText().Trim() = "ключевое слово" -> 
               href.Value() |> Some
            |_ -> None)
    |> Seq.tryHead

let getAllValues start  = 
    let rec loop (pages : string) = seq {
        let result = HtmlDocument.Load pages
        yield 
            result.Descendants "div"
            |> Seq.filter
                (fun node -> 
                    match node.TryGetAttribute "id" with
                    |Some id -> id.Value().StartsWith("текст для проверки")
                    |None -> false)
            |> Seq.map
                (fun node -> node.InnerText())
        let next = getNextLink result
        if next.IsSome then
            yield! loop next.Value

    }
    loop start

let path = "http://адрес.html"

let values =
    getAllValues path
    |> Seq.concat


остальная часть связана с обработкой полученных данных и к текущему вопросу отношения
не имеет.
Интересует как правильность выбора библиотеки для парсинга, так и сама реализация.
    

Ответы

Ответ 1

В целом функционально. Но если вы стремитесь к совершенству, то вот несколько советов:


Вся обработка происходит как бы вместе, в один присест. Весь код находится в одной
большой функции с подфункциями, и это делает код гораздо менее читаемым и запутанным.
Я обычно стараюсь разбить код на много мелких функций, и потом их друг с другом стыковать.
Идея в том, что каждая функция должна читаться на одном уровне, а не на нескольких.
Например "для каждого А вычислить Б", а не "для каждого А, где А - это все X из Y,
для которых выполняется Z, вычислить Б, где Б - это P или Q в зависимости от I + J".
Человечский мозг очень плохо справляется с частым переключением контекста.
Везде используются методы классов, что затрудняет вывод типов, заставляет указывать
типы явно, заставляет использовать лямбда-выражения. Я обычно стараюсь оборачивать
методы классов в мелкие функции, которые потом можно легко стыковать.
Очень мало параметризации, все строки зашиты в код. Если строки "id", "a" и "href"
ещё можно понять как часть стабильного стандарта, то строки "ключевое слово" и "текст
для проверки" совершенно точно должны быть параметрами.
Основной цикл у вас выдаёт последовательность последовательностей - seq>,
и потом вы её склеиваете с помощью Seq.concat. Эта операция видится мне лишней: раз
уж вы используете выражение seq { }, то можно внутри него сразу разворачивать последовательности
используя yield! вместо yield.
Здесь я не уверен, возможно это у вас такое требование, но: что происходит, если
на странице есть несколько ссылок с текстом "ключевое слово"? Ваша функция getNextLink
вернёт только первую ссылку, а остальные вы теряете. Можно было бы совсем без затрат
устроить обработку всех ссылок, а не только первой, просто убрав Seq.tryHead из getNextLink.
Но я не уверен, что это будет "лучше" в вашей ситуации.


Вот что у меня получилось в результате применения этих советов:

let attr name (node: HtmlNode) = node.TryGetAttribute name |> Option.map (fun v ->
v.Value())
let text (node: HtmlNode) = node.InnerText()
let trim (s: string) = s.Trim()
let descendants tag (node: #HtmlNode) = node.Descendants tag
let startsWith prefix (s: string) = s.StartsWith prefix

let hasText value node = trim (text node) = value
let hasId value node = attr "id" |> Option.exists (startsWith value)

let getNextLink text = 
    descendants "a"
    >> Seq.filter (hasText text)
    >> Seq.choose (attr "href")
    >> Seq.tryHead

let linksOnPage idPrefix doc =
    descendants "div" 
    >> Seq.filter (hasId idPrefix)
    >> Seq.map text

let getAllValues linkText idPrefix start  = 
    let rec linksFromPageTree (url : string) = 
        seq {
            let doc = HtmlDocument.Load url
            yield! linksOnPage idPrefix doc
            yield! nextSubTree doc
        }
    and nextSubTree doc = 
        match getNextLink linkText doc with
        | None -> Seq.empty
        | Some nextUrl -> linksFromPageTree nextUrl

    linksFromPageTree start

let path = "http://адрес.html"
let values = getAllValues "ключевое слово" "текст для проверки" path