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 recently wrote my first webapp and here is the code. I want to make it better, but I'm not exactly sure how. I believe the first place I should start is with the structure of it. Is this spaghetti code? If yes, how do I change that? You can view it at http://shiftfrog.com

I created a class called 'doer' that basically drives the program. Is that wrong? Should my class be called calendar and then manipulate in my post call to /calendar?

class Doer

  def makeDate(date)
    return Date.strptime(date, "%m/%d/%Y")
  end

  def buildArray(dateObj, on, off)
    array = frontpadding(dateObj, on, off)
    month = dateObj.month
    newMonth = month
    day = dateObj.mday
    date = dateObj
    while month == newMonth
      temp_array = [day,'day']
      array.push(temp_array)
      day = day + 1
      date = date + 1
      newMonth = date.month
    end
    endpadding(date, array)
    array
  end

  def endpadding(dateObj, array)
    dofw = dateObj.wday
    filler = (dofw - 6).abs + 1
    if filler == 7
      #do nothing
    else
      until filler == 0
        temp_array = ['','day']
        array.push(temp_array)
        filler = filler - 1
      end
    end
  end

  def frontpadding(dateObj, on, off)
    array = Array.new
    day = dateObj.mday 
    firstOfMonth = Date.new(dateObj.year, dateObj.month)
    filler = firstOfMonth.wday 
    on = on.to_i
    off = off.to_i
    mod = on + off
    if day != 1
      @days = day
      @location = @days + filler
      until day <= 1
        off.times do
          if day > 1
            temp_array = ['','day']
            array.unshift(temp_array)
            day = day - 1
          end
        end
        on.times do
          if day > 1
            temp_array = ['','dayOn']
            array.unshift(temp_array)
            day = day - 1
          end
        end
      end
    end 
    until filler == 0
        temp_array = ['','day']
        array.unshift(temp_array)
        filler = filler - 1
    end    
    array
  end

  def backFill(cal, on, off)
    @location = @location.to_i
    if @location > 0
      @location = @location - 1
      until @days == 0
        cal[0][1][@location][0] = @days
        @days = @days - 1
        @location = @location - 1
      end
    end
    cal
  end

  def makeCal(date, on, off)
    dateObj = makeDate(date)

    @cal = Array.new
    months = 0
    while months < 13
    #   #pass dateobj to build array
      array = buildArray(dateObj, on, off)
    #   #save array to hash with month key
      monthName = Date::MONTHNAMES[dateObj.mon]
      monthYear = "#{monthName} " + dateObj.year.to_s
      holder = Array.new
      holder.push monthYear
      holder.push array
      @cal.push holder
    #   #create new date object using month and set it to the first
      dateObj = Date.new(dateObj.year, dateObj.month)
      dateObj >>= 1
      months = months + 1
    end
    @cal = createCal(@cal, on, off)
    @cal = backFill(@cal, on, off)
  end

  def createCal(cal, on, off)
    on = on.to_i
    off = off.to_i
    @mod = on + off
    daycount = 0
    cal.each do |month|
      month[1].each do |day|
        if day[0] == ''
          #do nothing
        else
          if daycount % @mod < on
            day[1] = 'dayOn'          
          end
          daycount = daycount + 1 
        end
      end
    end       
    cal
  end
end

get '/' do
  haml :ask
end

post '/calendar' do
  @on = params["on"]
  @off = params["off"]
  @date = params["date"]


  a = Doer.new
  @cal = a.makeCal(@date, @on, @off)
  haml :feeling
end
share|improve this question
add comment

1 Answer

Doer is not a particularly expressive name. Sure, the class does something -- but what class doesn't? Your impulse to name the class Calendar is definitely on target. There are a few other naming things that are not according to Ruby style guidelines; you should avoid CamelCasing your method names -- for instance, makeCal should be make_cal and even better something like make! or create! (assuming the class itself is named calendar, saying @calendar.make_calendar is repeating yourself.)

In general, with respect to 'spaghetti' code, the issue is usually that you are not really separating concerns. The problem that Objects are trying to solve is encapsulating responsibility. It destroys any advantage you stand to gain to put all your functionality into one place. Object-oriented programming involves modularity and encapsulation -- naming a class Doer might indicate some weaknesses in terms of really grokking the paradigm. Consider studying some actual web application source code and see how concerns are separated and handled at an appropriate level of abstraction.

On that last point, one good rule of thumb is that code which is 'nearby' should be operating at roughly the same level of abstraction. Try to identify different concerns and modularize them -- make them 'loosely coupled' to one another. You might make a class which handles the business logic of padding and packing the string stuff, and separate the 'pure' calendar logic into a different class altogether; your application would then just have to 'glue' the two capabilities together.

share|improve this answer
add comment

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.