Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a method opening and appending to a file as so:

def writeFile(ofile,number)
  File.open(ofile, 'a') { |file| file.puts(number) }
end

Which is called after line processing by another method.

def method(ofile, ifile, number)
  File.foreach("#{ifile}") do |line|
    if number > 3
      writeFile(ofile, number) 
  end
end

My question is, should I just get rid of the writeFile method? It seems wrong to me. It keeps opening the file to write to it, when I should only open it once then write when needed.

share|improve this question

closed as off-topic by 200_success, syb0rg, tinstaafl, palacsint, Jeff Vanzella Feb 20 at 5:23

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – 200_success, syb0rg, tinstaafl, palacsint, Jeff Vanzella
If this question can be reworded to fit the rules in the help center, please edit the question.

1  
btw, it's more conventional in ruby to name methods like method_name rather than methodName. –  Nat Feb 16 '13 at 15:17
1  
As an aside, method is an existing method defined on Object, and you are trampling it. –  steenslag Feb 26 '13 at 22:15
1  
Welcome to Code Review. You have a conceptually good question, but it's not quite suitable for Code Review in its current form. We prefer to review actual code from a project that you are working on, rather than hypothetical code. Your if doesn't have a matching end. In addition, we may be able to provide more insightful advice if you don't decontextualize the code with generic method names like method(). I encourage you to edit the question to improve it. –  200_success Feb 20 at 2:53
add comment

1 Answer

up vote 3 down vote accepted

If this is the only place the writeFile method is called then you might as well put its single line into the other method in place of the method call.

As for the wisdom of appending to a file incrementally instead of building up a string to write all at once: It's a little extra work to repetitively open a file, but it might be worth it to to save on memory if you want to write a lot of data.

It very much depends on the nature of the data you're working with. It's best not to build up hundreds of MB of string(s) in memory if you can help it, but if you're just dealing with thousands of very small pieces of data (like numbers) then it's best not to open the file for each one.


EDIT:

that said you can also keep both files open the whole time like:

output = File.open(ofile, 'w')
File.foreach(ifile) { |line| output.puts line if line.length > 100 }
output.close

or

File.open(ofile, 'w') do |f|
  File.foreach(ifile) do |line|
    f.puts line if line.length > 100
  end
end
share|improve this answer
add comment

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