Merge #360

360: Adding Rect constructor to address issue #359: r=frewsxcv a=Chopinsky To address the issue #359 of lacking a Rect geo-type constructor that can perform coordinates validations, I've made the following changes: 1. Add a `new` constructor to the Rect geo-type. 2. Update all internal use of the Rect struct literals to the new constructor. However, existing use of the Rect struct literals in unit tests are left alone. Co-authored-by: Jacob Zuo <[email protected]>
georust · May 26, 2019 · 8805965 · 8805965
2 parents c0aa25f + 371cfde
commit 8805965
Show file tree

Hide file tree

Showing 3 changed files with 138 additions and 16 deletions.
diff --git a/geo-types/src/private_utils.rs b/geo-types/src/private_utils.rs
@@ -23,10 +23,8 @@ where
     let b = line.end;
     let (xmin, xmax) = if a.x <= b.x { (a.x, b.x) } else { (b.x, a.x) };
     let (ymin, ymax) = if a.y <= b.y { (a.y, b.y) } else { (b.y, a.y) };
-    Rect {
-        min: Coordinate { x: xmin, y: ymin },
-        max: Coordinate { x: xmax, y: ymax },
-    }
+
+    Rect::new(Coordinate { x: xmin, y: ymin }, Coordinate { x: xmax, y: ymax })
 }
 
 pub fn get_bounding_rect<I, T>(collection: I) -> Option<Rect<T>>
@@ -43,16 +41,17 @@ where
             xrange = get_min_max(px, xrange.0, xrange.1);
             yrange = get_min_max(py, yrange.0, yrange.1);
         }
-        return Some(Rect {
-            min: Coordinate {
+
+        return Some(Rect::new(
+            Coordinate {
                 x: xrange.0,
                 y: yrange.0,
             },
-            max: Coordinate {
+            Coordinate {
                 x: xrange.1,
                 y: yrange.1,
             },
-        });
+        ));
     }
     None
 }

diff --git a/geo-types/src/rect.rs b/geo-types/src/rect.rs
@@ -12,6 +12,41 @@ where
 }
 
 impl<T: CoordinateType> Rect<T> {
+    /// Constructor to creates a new rectangle from coordinates, where `min` denotes to the
+    /// coordinates of the bottom-right corner, and `max` denotes to the coordinates of the
+    /// top-left corner
+    ///
+    /// # Panics
+    ///
+    /// Panics if `min`'s x/y coordinate is larger than that of the `max`'s.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use geo_types::{Coordinate, Rect};
+    ///
+    /// let rect = Rect::new(
+    ///     Coordinate { x: 0., y: 0. },
+    ///     Coordinate { x: 10., y: 20. },
+    /// );
+    ///
+    /// assert_eq!(rect.min, Coordinate { x: 0., y: 0. });
+    /// assert_eq!(rect.max, Coordinate { x: 10., y: 20. });
+    /// ```
+    pub fn new<C>(min: C, max: C) -> Rect<T>
+    where
+        C: Into<Coordinate<T>>,
+    {
+        let (min, max) = (min.into(), max.into());
+
+        assert!(
+            min.x <= max.x && min.y <= max.y,
+            "Failed to create the Rect type: 'min' coordinate's x/y value must be smaller or equal to the 'max' x/y value"
+        );
+
+        Rect { min, max }
+    }
+
     pub fn width(self) -> T {
         self.max.x - self.min.x
     }
@@ -20,3 +55,34 @@ impl<T: CoordinateType> Rect<T> {
         self.max.y - self.min.y
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use crate::{Coordinate};
+
+    #[test]
+    fn rect() {
+        let rect = Rect::new((10, 10), (20, 20));
+        assert_eq!(rect.min, Coordinate { x: 10, y: 10 });
+        assert_eq!(rect.max, Coordinate { x: 20, y: 20 });
+    }
+
+    #[test]
+    #[should_panic]
+    fn rect_panic() {
+        let _ = Rect::new((10, 20), (20, 10));
+    }
+
+    #[test]
+    fn rect_width() {
+        let rect = Rect::new((10, 10), (20, 20));
+        assert_eq!(rect.width(), 10);
+    }
+
+    #[test]
+    fn rect_height() {
+        let rect = Rect::new((10., 10.), (20., 20.));
+        assert_eq!(rect.height(), 10.);
+    }
+}
diff --git a/geo/src/algorithm/map_coords.rs b/geo/src/algorithm/map_coords.rs
@@ -423,16 +423,18 @@ impl<T: CoordinateType, NT: CoordinateType> MapCoords<T, NT> for Rect<T> {
     fn map_coords(&self, func: &Fn(&(T, T)) -> (NT, NT)) -> Self::Output {
         let new_min = func(&(self.min.x, self.min.y));
         let new_max = func(&(self.max.x, self.max.y));
-        Rect {
-            min: Coordinate {
+
+        // using constructor to enforce soundness of the obtained coordinates
+        Rect::new(
+            Coordinate {
                 x: new_min.0,
                 y: new_min.1,
             },
-            max: Coordinate {
+            Coordinate {
                 x: new_max.0,
                 y: new_max.1,
             },
-        }
+        )
     }
 }
 
@@ -445,23 +447,29 @@ impl<T: CoordinateType, NT: CoordinateType> TryMapCoords<T, NT> for Rect<T> {
     ) -> Result<Self::Output, Error> {
         let new_min = func(&(self.min.x, self.min.y))?;
         let new_max = func(&(self.max.x, self.max.y))?;
-        Ok(Rect {
-            min: Coordinate {
+
+        Ok(Rect::new(
+            Coordinate {
                 x: new_min.0,
                 y: new_min.1,
             },
-            max: Coordinate {
+            Coordinate {
                 x: new_max.0,
                 y: new_max.1,
             },
-        })
+        ))
     }
 }
 
 impl<T: CoordinateType> MapCoordsInplace<T> for Rect<T> {
     fn map_coords_inplace(&mut self, func: &Fn(&(T, T)) -> (T, T)) {
         let new_min = func(&(self.min.x, self.min.y));
         let new_max = func(&(self.max.x, self.max.y));
+
+        // make sure the min coordinates are always smaller than the max coordinates, otherwise
+        // we won't have a "regular" rectangle that many of the methods assumes.
+        assert!(new_min.0 < new_max.0 && new_min.1 < new_max.1);
+
         self.min.x = new_min.0;
         self.min.y = new_min.1;
         self.max.x = new_max.0;
@@ -490,6 +498,55 @@ mod test {
         assert_eq!(p2.y(), 110.);
     }
 
+    #[test]
+    fn rect_inplace() {
+        let mut rect = Rect::new((10, 10), (20, 20));
+        rect.map_coords_inplace(&|&(x, y)| (x + 10, y + 20));
+        assert_eq!(rect.min, Coordinate { x: 20, y: 30 });
+        assert_eq!(rect.max, Coordinate { x: 30, y: 40 });
+    }
+
+    #[test]
+    #[should_panic]
+    fn rect_inplace_panic() {
+        let mut rect = Rect::new((10, 10), (20, 20));
+        rect.map_coords_inplace(&|&(x, y)| {
+            if x < 15 && y < 15 {
+                (x, y)
+            } else {
+                (x - 15, y - 15 )
+            }
+        });
+    }
+
+    #[test]
+    fn rect_map_coords() {
+        let rect = Rect::new((10, 10), (20, 20));
+        let another_rect = rect.map_coords(&|&(x, y)| (x + 10, y + 20));
+        assert_eq!(another_rect.min, Coordinate { x: 20, y: 30 });
+        assert_eq!(another_rect.max, Coordinate { x: 30, y: 40 });
+    }
+
+    #[test]
+    fn rect_try_map_coords() {
+        let rect = Rect::new((10, 10), (20, 20));
+        let result = rect.try_map_coords(&|&(x, y)| Ok((x + 10, y + 20)));
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    #[should_panic]
+    fn rect_try_map_coords_panic() {
+        let rect = Rect::new((10, 10), (20, 20));
+        let _ = rect.try_map_coords(&|&(x, y)| {
+            if x < 15 && y < 15 {
+                Ok((x, y))
+            } else {
+                Ok((x - 15, y - 15))
+            }
+        });
+    }
+
     #[test]
     fn line() {
         let line = Line::from([(0., 0.), (1., 2.)]);