On Mon, Mar 23, 2015 at 5:31 PM, Robert Klemme <shortcutter / googlemail.com>
wrote:

>
>
> On Mon, Mar 23, 2015 at 3:43 PM, leam hall <leamhall / gmail.com> wrote:
>
>> My script is running over an XML file with Nokogiri. There are
>> currently two files it goes over and the process for both is the same.
>> Thus, a perfect candidate for a method.
>>
>> What I seem to be having issues with is scope. Each file lists
>> security vulnerabilities and has details about them. I create a main
>> hash that has each unique vulnerability ID as a nested hash. What
>> failed was referring to the main hash inside the method.
>>
>
> I would not do that. Instead, I would let the method return a Hash with
> all the vulnerabilities in the current file. If you need to combine them,
> you can still use Hash#merge afterwards.  Alternatively, pass in the global
> Hash for updating.
>
> I would also define a class for a single vulnerability because fields seem
> to be the same all the time and you can add more methods to that class if
> need be.  The simplest thing you can do is
>
> Vulnerability = Struct.new :vuln_number, :title, :severity
>
> f5 = File.open(infile5)
>>
>> doc5 = Nokogiri::XML(f5)
>>
>
> You are not properly closing the file.  Better do this and be safe even in
> case of IO or parsing errors:
>
> doc5 = File.open(infile5) {|f5| Nokogiri::XML(f5)}
>
> vulns = Hash.new
>>
>> doc5.xpath('//Group').each do  |group|
>>   vuln_number = group.attributes['id']
>>   unless vulns.include? vuln_number
>>     vulns[vuln_number] = Hash.new
>>     vulns[vuln_number]['title'] = group.xpath('Rule/title').text
>>     vulns[vuln_number]['severity'] = group.xpath('Rule').attr('severity')
>>   end
>> end
>>
>
> It is more efficient to create the per vulnerability Hash locally, fill it
> and then set it in the Hash.
>

Meant to say: more efficient _and more robust_. Reason is, that should
something fail there won't be a half constructed vulnerability Hash in the
overall Hash.


> unless vulns[vuln_number]
>   vuln = {:vuln_number => vuln_number}
>   vuln['title'] = group.xpath('Rule/title').text
>   vuln['severity'] = group.xpath('Rule').attr('severity')
>   vulns[vuln_number] = vuln
> end
>
> Or even a bit more arcane
>
> vulns[vuln_number] || = {:vuln_number => vuln_number}.tap do |vuln|
>   vuln['title'] = group.xpath('Rule/title').text
>   vuln['severity'] = group.xpath('Rule').attr('severity')
> end
>
> You will note that I always included the vuln_number in the Hash or
> Vulnerability as it is an identifying element of a vulnerability that won't
> get lost if you only look at a single Hash / Vulnerability.
>
> Cheers
>
> robert
>
>
> --
> [guy, jim, charlie].each {|him| remember.him do |as, often| as.you_can -
> without end}
> http://blog.rubybestpractices.com/
>



-- 
[guy, jim, charlie].each {|him| remember.him do |as, often| as.you_can -
without end}
http://blog.rubybestpractices.com/