Pr 4: find_xml_file #5
Conversation
|
|
||
| files.each do |filename| | ||
| if filename.match "\.xml$" | ||
| xml_file = filename | ||
| break | ||
| end | ||
| end |
There was a problem hiding this comment.
This will only return the first xml file in the provided folder. Consider the case that multiple .xml files might exist.
| files.each do |filename| | ||
| if filename.match "\.xml$" | ||
| xml_file = filename | ||
| break | ||
| end | ||
| end |
There was a problem hiding this comment.
| files.each do |filename| | |
| if filename.match "\.xml$" | |
| xml_file = filename | |
| break | |
| end | |
| end | |
| xml_file = files.find{|filename| filename.match "\.xml$"} |
There was a problem hiding this comment.
Consider using the .find method to make the code more elegant and readable.
|
|
||
| def find_xml_file(folder) | ||
| files = Dir.entries(folder) | ||
|
|
There was a problem hiding this comment.
I think this code works. I wonder if there is a way to use .include in place of a .each loop unless there is a reason for choosing to use an .each loop? If so, would love to hear why for future reference!
|
|
||
| describe "find_xml_file" do | ||
| it "should return nil if there are no xml files" do | ||
| expect(find_xml_file('.')).must_be_nil |
There was a problem hiding this comment.
Maybe we can add another test case or change this to a directory without an xml file.
| Rake::TestTask.new do |t| | ||
| t.libs = ["lib"] | ||
| t.warning = true | ||
| t.test_files = FileList['test/*_test.rb'] | ||
| end |
| files.each do |filename| | ||
| if filename.match "\.xml$" | ||
| xml_file = filename | ||
| break | ||
| end |
There was a problem hiding this comment.
Consider using an if else block in the place of break
| @@ -0,0 +1,10 @@ | |||
| require 'rake/testtask' | |||
|
|
|||
| Rake::TestTask.new do |t| | |||
There was a problem hiding this comment.
Use a more descriptive name than 't' to help with clarity
| require 'minitest/reporters' | ||
| require 'minitest/skip_dsl' | ||
| require 'awesome_print' | ||
| # Add simplecov |
There was a problem hiding this comment.
it looks like you forgot to add require 'simplecov' to this test_helper.rb
| describe "find_xml_file" do | ||
| it "should return nil if there are no xml files" do | ||
| expect(find_xml_file('.')).must_be_nil |
There was a problem hiding this comment.
Consider adding a couple more test cases
| def find_xml_file(folder) | ||
| files = Dir.entries(folder) | ||
|
|
||
| xml_file = nil |
There was a problem hiding this comment.
Is there a chance there might be more than 1 .xml file in the directory being searched? if so turning nil into an array might be helpful.
| expect(find_xml_file('.')).must_be_nil | ||
| end | ||
|
|
||
| it "should return an xml file if it's in the folder" do |
There was a problem hiding this comment.
If the above nil does become an array, don't forget to adjust test accordingly. :-)
| describe "find_xml_file" do | ||
| it "should return nil if there are no xml files" do | ||
| expect(find_xml_file('.')).must_be_nil | ||
| end | ||
|
|
||
| it "should return an xml file if it's in the folder" do | ||
| expect(find_xml_file('./test/test_data')).must_equal "example.xml" | ||
| end | ||
| end |
There was a problem hiding this comment.
Consider: adding a test for multiple XML files.
No description provided.