current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

I'm trying to decide if I should put time into refactoring this tiny class. PathParser only uses string locally and every other method is used by an outside class. Should I put the small bits of logic into initialize or keep this as is?

class PathParser

  attr_reader :str

  def initialize(str)
    @str = str
  end

  def start_point 
    str.index('#')
  end

  def end_point
    str.rindex('#')
  end

  def row_length
    str.index("\n") + 1
  end

  def vertical_path_length
    (end_point / row_length) - (start_point / row_length)
  end
end
share|improve this question
5  
What is the intended purpose of this class? Could you update the question to include an example string and usage? –  200_success♦ Feb 21 at 7:15

1 Answer 1

up vote 3 down vote

Although I am not a Ruby guy, as a general rule of thumb I would suggest to keep it as is, since none of your accessor methods change the state of the PathParser object. The underlying string is always the same and the accessors only return relevant information based on the original state.

Essentially its a common trade off between memory and CPU, do you want to calculate everything at once and hold a bigger object in memory or you are ok with calculating on the fly and keeping a smaller object in memory. If the PathParser is initialized relatively less frequently as opposed to how often the different checks on the string are performed, then I would suggest maybe moving the calculations it in the constructor.

Hope that helps, again I come from a Java background, so my opinion might not be idiomatic to Ruby.

share|improve this answer
3  
You can get the best of both worlds with memoization. For example str.index('#') would become @start_point ||= str.index('#'). The first time start_point is called it will calculate the starting point, after that it will return the cached value. This should only be used if you're worried about calculating the same value a lot, as it will consume memory. –  britishtea Feb 21 at 14:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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