patternrubyMinor
Is this Spaghetti code?
Viewed 0 times
thiscodespaghetti
Problem
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
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
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
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
Solution
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.
Context
StackExchange Code Review Q#4740, answer score: 6
Revisions (0)
No revisions yet.