From 7219198524e116c9a5aee8f357465e3985176d16 Mon Sep 17 00:00:00 2001
From: Kenneth Gitere <gitere81@gmail.com>
Date: Wed, 23 Sep 2020 22:36:01 +0300
Subject: [PATCH] Change function signature of `next_element` to return an
 Option rather than mutate a given value.

The new function signature reads a little easier than before.
Remove TODO task in replace_brs
---
 src/moz_readability/mod.rs | 40 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/src/moz_readability/mod.rs b/src/moz_readability/mod.rs
index b17a566..a5823d2 100644
--- a/src/moz_readability/mod.rs
+++ b/src/moz_readability/mod.rs
@@ -143,16 +143,9 @@ impl Readability {
                                 .borrow_mut()
                                 .insert(attr_name, prev_value.value.clone());
                         }
-                        // TODO: Replace the code below with next_element
-                        prev_elem.insert_after(
-                            inner_node_ref
-                                .first_child()
-                                .unwrap()
-                                .children()
-                                .filter(Self::has_content)
-                                .next()
-                                .unwrap(),
-                        );
+
+                        let inner_node_child = Self::next_element(inner_node_ref.first_child());
+                        prev_elem.insert_after(inner_node_child.unwrap());
                         prev_elem.detach();
                     }
                 }
@@ -193,11 +186,9 @@ impl Readability {
     ///   <div>foo<br>bar<p>abc</p></div>
     fn replace_brs(&mut self) {
         if let Ok(mut br_tags) = self.root_node.select("br") {
-            // TODO: Change to for loop
             while let Some(br_tag) = br_tags.next() {
-                let mut next = br_tag.as_node().next_sibling();
+                let mut next = Self::next_element(br_tag.as_node().next_sibling());
                 let mut replaced = false;
-                Self::next_element(&mut next);
                 while let Some(next_elem) = next {
                     if next_elem.as_element().is_some()
                         && &next_elem.as_element().as_ref().unwrap().name.local == "br"
@@ -205,11 +196,10 @@ impl Readability {
                         replaced = true;
                         let br_sibling = next_elem.next_sibling();
                         next_elem.detach();
-                        next = br_sibling;
+                        next = Self::next_element(br_sibling);
                     } else {
                         break;
                     }
-                    Self::next_element(&mut next);
                 }
                 if replaced {
                     let p = NodeRef::new_element(
@@ -309,22 +299,21 @@ impl Readability {
     /// Finds the next element, starting from the given node, and ignoring
     /// whitespace in between. If the given node is an element, the same node is
     /// returned.
-    fn next_element(node_ref: &mut Option<NodeRef>) {
-        // The signature of this method affects how it is used in loops since the code
-        // has to ensure that it is called before the next iteration. This probably
-        // makes it less obvious to understand the first time. This may change in the future.
+    fn next_element(node_ref: Option<NodeRef>) -> Option<NodeRef> {
+        let mut node_ref = node_ref;
         while node_ref.is_some() {
             match node_ref.as_ref().unwrap().data() {
                 NodeData::Element(_) => break,
                 _ => {
                     if node_ref.as_ref().unwrap().text_contents().trim().is_empty() {
-                        *node_ref = node_ref.as_ref().unwrap().next_sibling();
+                        node_ref = node_ref.as_ref().unwrap().next_sibling();
                     } else {
                         break;
                     }
                 }
             }
         }
+        node_ref
     }
 
     /// Determine if a node qualifies as phrasing content.
@@ -433,7 +422,7 @@ mod test {
         let p = doc.root_node.select_first("#a").unwrap();
         let p = p.as_node();
         let mut p_node_option: Option<NodeRef> = Some(p.clone());
-        Readability::next_element(&mut p_node_option);
+        p_node_option = Readability::next_element(p_node_option);
         assert_eq!(Some(p.clone()), p_node_option);
 
         let p_node_option = p_node_option.unwrap();
@@ -441,23 +430,20 @@ mod test {
         let p_node_option_attr = p_node_option.unwrap().attributes.borrow();
         assert_eq!("a", p_node_option_attr.get("id").unwrap());
 
-        let mut next = p.next_sibling();
-        Readability::next_element(&mut next);
+        let next = Readability::next_element(p.next_sibling());
 
         let next = next.unwrap();
         let next_elem = next.as_element();
         let next_attr = next_elem.unwrap().attributes.borrow();
         assert_eq!("b", next_attr.get("id").unwrap());
 
-        let mut next = next.next_sibling();
-        Readability::next_element(&mut next);
+        let next = Readability::next_element(next.next_sibling());
 
         let next = next.unwrap();
         assert_eq!(true, next.as_text().is_some());
         assert_eq!("This is standalone text", next.text_contents().trim());
 
-        let mut next = None;
-        Readability::next_element(&mut next);
+        let next = Readability::next_element(None);
         assert_eq!(None, next);
     }