1
\$\begingroup\$

I have a table containing about 30k rows of data and there's several table associations. The problem is that there's like 18 columns, and each column requires 1 db query.

Here's an example of what I'm doing:

if col_num == 0
    tmp_string_text = finding_phase
elsif col_num == 1
    tmp_string_text = ""
elsif col_num == 2
    tmp_string_text = ""
elsif col_num == 3
    tmp_string_text =  finding.vuln_finding.severity
elsif col_num == 4
    tmp_string_text = finding.node.ip
elsif col_num == 5
    tmp_string_text =  finding.node.host_name
elsif col_num == 6
    tmp_string_text =  finding.node.dns_name
elsif col_num == 7
    tmp_string_text = finding.port
elsif col_num == 8
    tmp_string_text =  finding.pentest_finding.name
elsif col_num == 9
    tmp_string_text =  finding.vuln_finding.name
elsif col_num == 10
    tmp_string_text = finding.vuln_finding.description
elsif col_num == 11
    tmp_string_text = finding.vuln_finding.solution
elsif col_num == 12
    tmp_string_text = finding.additional_output
elsif col_num == 13
    tmp_string_text = finding.cve
elsif col_num == 14
    tmp_string_text = finding.node.os
elsif col_num == 15
    tmp_string_text =  finding.node.device_type
elsif col_num == 16
    tmp_string_text =  finding.node.scan_time
end

As a result, there's going to be 15 queries to the DB per 1 row. Is there any way that I can make this more efficient? This takes a REALLY long time when I have 30k rows. Not looking for anyone to flame here, just looking for advice. Pretty new to Ruby on Rails but learning as I go.

Basically, "finding" in this case is a result of @my_report.findings and it's going through each finding and pulling back data.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! As a courtesy to other users, please declare your cross-posts. \$\endgroup\$ – 200_success Jan 21 '16 at 22:23
  • 2
    \$\begingroup\$ Stack Overflow can reformulate your code, but Code Review would prefer to give you the best possible advice for your situation. Please tell us more about what you are trying to accomplish. Is there a reason for this particular column numbering? What is tmp_string_text for? What is the finding object? What about finding_node_ip — is that a function call, a method call, or a variable? Better yet, show your entire function or class, so that we have more context. \$\endgroup\$ – 200_success Jan 21 '16 at 22:30
  • \$\begingroup\$ Thanks. So tmp_string_text is the data that will go in that column. I'm iterating through a bunch of columns and if, for example, the column == 1, then tmp_string_text gets assigned that text. At the bottom of this function, I'm doing something with tmp_string_text (inserting it into a cell. \$\endgroup\$ – LewlSauce Jan 21 '16 at 22:48
  • \$\begingroup\$ But your "explanation" just reiterates what the code says, and doesn't answer my questions. \$\endgroup\$ – 200_success Jan 21 '16 at 22:54
  • \$\begingroup\$ Long story short, tmp_string_text holds data based on the colum number. finding_node_ip = finding.node.ip. Ultimately, as opposed to calling finding.node.os, finding.node.device_type, finding.node.scan_time, finding.vuln_finding.description, etc, I'm wondering if there's a way to call all of the required information in one call. Sorry. Let me know if that helps. \$\endgroup\$ – LewlSauce Jan 21 '16 at 23:06
1
\$\begingroup\$

When I see long lists of if/elsif, I look to see whether case/when is a good alternate:

tmp_string_text = case col_num
                  when 0
                    finding_phase
                  when 1, 2
                    ""
                  when 3
                    finding.vuln_finding.severity
                  when 4
                    finding.node.ip
                  when 5
                    finding.node.host_name
                  when 6
                    finding.node.dns_name
                  when 7
                    finding.port
                  when 8
                    finding.pentest_finding.name
                  when 9
                    finding.vuln_finding.name
                  when 10
                    finding.vuln_finding.description
                  when 11
                    finding.vuln_finding.solution
                  when 12
                    finding.additional_output
                  when 13
                    finding.cve
                  when 14
                    finding.node.os
                  when 15
                    finding.node.device_type
                  when 16
                    finding.node.scan_time
                  end

In this case I think it's a reasonable solution. If you were only returning strings then a hash would be even more compact:

tmp_string_text = {
  0 => 'finding_phase',
  1 => "",
  2 => "",
  3 => 'finding.vuln_finding.severity',
}[col_num]

That's obviously truncated, but you get the idea.

But, again, you're executing code for your return value once your condition matches. Trying to use a hash will cause all that code to execute when the hash is initialized, which is likely to not be what you want.

As an exercise, I tried clarifying what's going on by using nested case statements:

tmp_string_text = case col_num
                  when 0
                    finding_phase
                  when 1 .. 2
                    ""
                  when 3
                    finding.vuln_finding.severity
                  when 4 .. 6
                    meth = case col_num
                          when 4
                            :ip
                          when 5
                            :host_name
                          when 6
                            :dns_name
                          end
                    finding.node.send(meth)
                  when 7
                    finding.port
                  when 8
                    finding.pentest_finding.name
                  when 9 .. 11
                    meth = case col_num
                          when 9
                            :name
                          when 10
                            :description
                          when 11
                            :solution
                          end
                    finding.vuln_finding.send(meth)
                  when 12
                    finding.additional_output
                  when 13
                    finding.cve
                  when 14 .. 16
                    meth = case col_num
                          when 14
                            :os
                          when 15
                            :device_type
                          when 16
                            :scan_time
                          end
                    finding.node.send(meth)
                  end

Obviously that's untested. It's a bit longer, but the nesting does help reveal the logic.

Then, as a DRY exercise, took that and tried to reduce the repetition a bit:

case col_num
when 0
  finding_phase
when 1 .. 2
  ""
when 3 .. 16

  result = finding

  methods = case col_num
            when 3
              [:vuln_finding, :severity]
            when 4 .. 6
              [:node] << case col_num
              when 4
                :ip
              when 5
                :host_name
              when 6
                :dns_name
              end
            when 7
              [:port]
            when 8
              [:pentest_finding, :name]
            when 9 .. 11
              [:vuln_finding] << case col_num
              when 9
                :name
              when 10
                :description
              when 11
                :solution
              end
            when 12
              [:additional_output]
            when 13
              [:cve]
            when 14 .. 16
              [:node] << case col_num
              when 14
                :os
              when 15
                :device_type
              when 16
                :scan_time
              end
            end
  methods.each do |m|
    result = result.send(m)
  end
  result
end

Again that's not tested. It loses some readability at that point but it's pretty easy to figure out what's going on.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ You should read codereview.stackexchange.com/help/someone-answers. \$\endgroup\$ – the Tin Man Jan 22 '16 at 1:17
  • \$\begingroup\$ @theTinMan They look basically the same, except one is a line longer... \$\endgroup\$ – Bam Jan 25 '16 at 21:03
  • \$\begingroup\$ That's because the current starting code has little repetition in the tests. Note that 1, 2 have been combined where they can't be using if-based code. case/when has a lot of potential to reduce long conditional tests greatly. \$\endgroup\$ – the Tin Man Jan 25 '16 at 21:07
  • 1
    \$\begingroup\$ Something to consider also is it's not the number of lines that's important, it's the readability and whether it's DRY enough. I'd much rather maintain some code that's a little verbose but very readable than code I have to spend 30 minutes decoding before I understand it. \$\endgroup\$ – the Tin Man Jan 25 '16 at 21:58

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.