Repository: jaytaph/gosub-browser
File: src/html5_parser/element_class.rs (Rust)

Code before:

impl ElementClass {
    /// Initialise a new (empty) ElementClass
    pub fn new() -> Self {
        ElementClass {
            class_map: HashMap::new(),
        }
    }

    /// Initialize a class from a class string
    /// with space-delimited class names
    pub fn from_string(class_string: &str) -> Self {
        let mut class_map_local = HashMap::new();
        let classes = class_string.split_whitespace();
        for class_name in classes {
            class_map_local.insert(class_name.to_owned(), true);
        }

        ElementClass {
            class_map: class_map_local,
        }
    }

    /// Count the number of classes (active or inactive)
    /// assigned to an element
    pub fn count(&self) -> usize {
        self.class_map.len()
    }

    /// Check if any classes are present
    pub fn is_empty(&self) -> bool {
        self.class_map.is_empty()
    }

    /// Check if class name exists
    pub fn contains(&self, name: &str) -> bool {
        self.class_map.contains_key(name)
    }

    /// Add a new class (if already exists, does nothing)
    pub fn add(&mut self, name: &str) {
        // by default, adding a new class will be active.
        // however, map.insert will update a key if it exists
        // and we don't want to overwrite an inactive class to make it active unintentionally
        // so we ignore this operation if the class already exists
        if !self.contains(name) {
            self.class_map.insert(name.to_owned(), true);
        }
    }

    /// Remove a class (does nothing if not exists)

Review comment (suggestion):
Wouldn't it be better to stick with the Rust style here and use `len` as the method name? Especially because there is an `is_empty` method, which gets suggested by clippy when doing any comparison to zero, e.g. `list.len() == 0`.

Code after:

impl ElementClass {
    /// Initialise a new (empty) ElementClass
    pub fn new() -> Self {
        ElementClass {
            class_map: HashMap::new(),
        }
    }

    /// Initialize a class from a class string
    /// with space-delimited class names
    pub fn from_string(class_string: &str) -> Self {
        let mut class_map_local = HashMap::new();
        let classes = class_string.split_whitespace();
        for class_name in classes {
            class_map_local.insert(class_name.to_owned(), true);
        }

        ElementClass {
            class_map: class_map_local,
        }
    }

    /// Count the number of classes (active or inactive)
    /// assigned to an element
    pub fn len(&self) -> usize {
        self.class_map.len()
    }

    /// Check if any classes are present
    pub fn is_empty(&self) -> bool {
        self.class_map.is_empty()
    }

    /// Check if class name exists
    pub fn contains(&self, name: &str) -> bool {
        self.class_map.contains_key(name)
    }

    /// Add a new class (if already exists, does nothing)
    pub fn add(&mut self, name: &str) {
        // by default, adding a new class will be active.
        // however, map.insert will update a key if it exists
        // and we don't want to overwrite an inactive class to make it active unintentionally
        // so we ignore this operation if the class already exists
        if !self.contains(name) {
            self.class_map.insert(name.to_owned(), true);
        }
    }

    /// Remove a class (does nothing if not exists)