0
\$\begingroup\$

I have a dataset that looks something like this:

{
   "project":{
      "components":[
         {
            "name":"1",
            "description":"1"
         },
         {
            "name":"1-subitem1",
            "description":"1A"
         },
         {
            "name":"1-subitem2",
            "description":"1B"
         },
         {
            "name":"2",
            "description":"2"
         },
         {
            "name":"2-subitem1",
            "description":"2A"
         },
         {
            "name":"3",
            "description":"3"
         }
      ]
   }
}

And I've created a tree view list of the components in ascending order by description with the letter defining a subitem, so it looks something like this:

  • 1, 1
    • 1-subitem1, 1A
    • 1-subitem2, 1B
  • 2, 2
    • 2-subitem1, 2A
  • 3, 3

So I've successfully done that just fine, but I want to make sure I'm doing it in the most Ruby-oriented way possible.

I've created a partial called layouts/navigation/_tree_view.html.erb. This will be included in almost all views.

<% if defined? project %>
    <%  
        components = retrieve_components(project)
        previous_component = {}
        components.each do |component| 
    %>

    <%= open_or_close_unordered_list(previous_component, component).html_safe %>

    <% if !containsLetter(component.description) %>
        <li>
            <a href="#">
                <i class="fa <%= $component_symbol_hash[component.name] %>"></i>
                <span class="nav-label"><%= truncate(component.name, length: 20) %></span>
                <span class="fa arrow"></span>
            </a>
    <% else %>
        <li class=""><a href="#"><%= component.name %></a></li>
    <% end %>

    <% previous_component = component %>
    <% end %>
<% end %>

Because this is going to be throughout my application views, I put the helper methods in my ApplicationHelper

module ApplicationHelper

    $component_symbol_hash = {
      '1'     => "fa-check-square",
      '2'     => "fa-list",
      '3'     => "fa-cogs"
    }

    # Determine if subcomponent
    def containsLetter(string_value) 
      string_value.count("a-zA-Z") > 0 ? true : false
    end

    # Remove component with description "General" from components
    def remove_general_component(components)
      index_of_general_component = components.index{ |item| item.name == "General" }
      components.delete_at(index_of_general_component)
      return components
    end

    # Sort components by description in desending order
    def sort_components_by_description(components)
      components = (components).sort! { |a, b|  a.description <=> b.description }
    end

    # Retrieve components from project in desired format
    def retrieve_components(project)
      components = project.components
      components = sort_components_by_description(components)
      components = remove_general_component(components)
    end

    def open_or_close_unordered_list(previous_component, current_component)
      # If the current component isn't a subcomponent AND the previous component was a subcomponent, end ul
      if previous_component != {} and !containsLetter(current_component.description) and containsLetter(previous_component.description)
        "</li></ul>"

      # If the current component is a subcomponent AND the previous component wasn't, start ul
      elsif containsLetter(current_component.description) and !containsLetter(previous_component.description)
        "<ul class=\"nav nav-second-level collapse\">"

      # Else return nothing
      else
        "</li>"
      end
    end

end

The output of my logic in HTML becomes:

<li class="">
    <a href="#">
        <i class="fa fa-check-square"></i>
        <span class="nav-label">1, 1</span>
        <span class="fa arrow"></span>
    </a>
    <ul class="nav nav-second-level collapse" style="height: 0px;">
        <li class=""><a href="#">1-subitem1, 1A</a></li>
        <li class=""><a href="#">1-subitem2, 1B</a></li>
    </ul>
</li>

<li class="">
    <a href="#">
        <i class="fa fa-list"></i>
        <span class="nav-label">2, 2</span>
        <span class="fa arrow"></span>
    </a>
    <ul class="nav nav-second-level collapse" style="height: 0px;">
        <li class=""><a href="#">2-subitem1, 2A</a></li>
    </ul>
</li>

<li class="">
    <a href="#">
        <i class="fa fa-cogs"></i>
        <span class="nav-label">3, 3</span>
        <span class="fa arrow"></span>
    </a>
</li>

Should this logic be somewhere else other than my ApplicationHelper? I feel like it should.

Additionally, is there something I should be doing to optomize my logic?

\$\endgroup\$
2
\$\begingroup\$

I have a couple of suggestions on the detail, but most of all it seems that most of the code is really in the wrong place, and is quite procedural.

Example: you effectively use description.count("a-zA-Z") > 0 ? true : false (which should just be description.count("a-zA-Z") > 0) and add a code comment to say "is this a subcomponent?". Instead, add a method to the Component class ...

def subcomponent?
  description.count("a-zA-Z") > 0
end

... and maybe another for ...

def not_subcomponent?
  !subcomponent?
end

... so instead of ...

!containsLetter(current_component.description) and containsLetter(previous_component.description)

... you can ...

current_component.not_subcomponent? && previous_component.subcomponent?

Do you see how that improves the code? Only the Component needs to know how to determine whether it is a subcomponent or not.

Similarly, you have code that does ... |item| item.name == "General" .... Instead, the component have the knowledge of whether it is a general component or not, and this lets the project return a set of components that are not general. You can do this by defining a scope on Component and merging that scope into an association between Project and Component, so you can call project.non_general_components.

Look into using the <=> operator to define an default sort order on components based on their description so you can also call project.non_general_components.sort. Only the component class should know how it should be sorted, so encapsulate the logic in there.

Long-story-short, I think more encapsulation of logic within the Project and Component classes will help you greatly. It will also make it much easier to test the logic.

\$\endgroup\$
  • \$\begingroup\$ Component is not something that I'm defining, it's from a JSON object I'm consuming from an API. \$\endgroup\$ – beckah Aug 14 '17 at 20:52
  • \$\begingroup\$ @beckah remember though that everything is an object, and if the structure you're using doesn't support what you want to do then you can add behaviour to it by defining a new class that does. Arguably, this is even more important with an API response, when the structure and behaviour is out of your control, because you can centralise your interpretation of the API values into a single class, instead of potentially having to change it throughout your code. \$\endgroup\$ – David Aldridge Aug 15 '17 at 16:34
  • \$\begingroup\$ true.. didn't really think about this. So essentially the API i'm consuming should be stored within it's own class local to my project and I should extend that class to have the methods you've described. Didn't really think about this (kind of a newb tbh), but that's an awesome idea. Thank you so much for your help. \$\endgroup\$ – beckah Aug 15 '17 at 17:54
  • \$\begingroup\$ Yes, I think that would be a good move. 3rd party APIs in particular are really prone to unannounced breaking changes, so do whatever you can to insulate your code from them. \$\endgroup\$ – David Aldridge Aug 15 '17 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.