2
\$\begingroup\$

Is there a better way of doing this?

File.open( "/etc/my.cnf" ) do |f|
  f.grep( /^testdir/ ) do |line|
    test1 = line.chop.gsub(/ /,"")
    test2 = test1.sub(/.*=/, "")
    RETURN_PATH = "#{test2}/test_ups"
  end
end
\$\endgroup\$
3
  • 3
    \$\begingroup\$ What's wrong with this one? What do you want to improve? Define better. \$\endgroup\$
    – Martinho Fernandes
    Commented Apr 6, 2011 at 14:30
  • \$\begingroup\$ It works fine but still wants to know if any other efficient way is available. \$\endgroup\$
    – krunal shah
    Commented Apr 6, 2011 at 14:33
  • 2
    \$\begingroup\$ I'm ok with these kinds of questions on SO as long as there is a good definition of better. Like Douglas suggests, the people at codereview.SE have less strict standards for questions having definite answers (they will comment any code on any aspect), and they need the questions to take the site out of beta (I hope they make it, seems like a very good addition to SE). \$\endgroup\$
    – Martinho Fernandes
    Commented Apr 6, 2011 at 14:47

3 Answers 3

3
\$\begingroup\$

Aside from not using a constant for RETURN_PATH (which is obviously wrong and will cause Ruby to warn you), it seems reasonable enough to me.

\$\endgroup\$
3
\$\begingroup\$

Here's my refactoring, I'll explain below what I've done:

path = File.readlines("/etc/my.cnf").find { |line| line =~ /^testdir/ }.chomp.gsub(/( )|(.*=)/, '')
@return_path = File.join(path, 'test_ups') # >> "/tmp/test_ups"

Firstly you can use File.readlines to return an Array of lines from your file, we then use your regex to match a single line beginning with testdir, we then remove the \r\n with chomp - notice I changed chop for chomp because it is safer (Refer to Ruby#String) and then clean it up with a single regex and gsub. Lastly we return your path in the instance variable @return_path rather than a constant.

I've made a few assumptions when writing this, including my.cnf being the standard MySQL conf file - you may need to adapt this to suit.

\$\endgroup\$
1
\$\begingroup\$

I'm not sure what the semantics of your code are, but on the assumption that you are munging things that look like paths to files, I would commend to you the File.basename method (and friends) that might express more cleanly your intent. Note: there is no need to actually access the file to get the basename. It is a simple string operation.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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