Feedback on my design and how to use DCI Design Pattern?

[josh]

I would like to get your feedback on the initial design of my app.
The goal is to email a user when an event he cares about change it's time.
i have User class, Event Class and an Email module.

some questions:
1. should Email be a class instead of a module?
2. should Email be refactored into a method of the User class?
3. I read a bit about DCI Design Pattern (http://www.clean-ruby.com/ and http://saturnflyer.com/blog/jim/201...what-your-system-is-vs-what-your-system-does/) and I am curious if there is a way to use it here.
In DCI I believe I'll need a context module that will describe the main use cases of my app,
and it each use case i'll add roles to some objects. in my case the user object will be extended with Email module (the Email will turn into a role, being able to send emails).

I pasted my code below but it's nicer here: http://pastebin.com/XA6ez8FY

# email a user when an event's time was changed.
#
# features
# 1. user can add events (each event has a known url).
# 2. every morning if any event's time was changed,
# the user gets email with the changed events.
# 3. on Monday morning, user gets email of all his events
# regardless of changes in times.

# holds a user and it's events
class User
attr_accessor :events, :name

def initialize name
@name = name
@events = nil
@updated = false
end

# ask each event to get it's current time and
# mark the user as 'updated' so later on we
# will email him/her
def get_time
@updated = @events.any?{|e| e.get_time; e.updated?}
end

def updated?
@updated
end
end

# holds info about a event
class Event
attr_accessor :name, :url, :base_time

def initialize name, url, base_time
@name = name
@url = url
@base_time = base_time
@current_time = nil
@updated = false
end

# scrape event from the website and mark it
# as 'updated' if it's time was modified
def get_time
current_time = fetch_current_time
@current_current_time = current_time if current_time
if current_time != @base_time
@updated = true
end
end

def updated?
@updated
end

private

# get current event time from the website
def fetch_current_time

123124234324
end
end

# send email to a user
module Email
module_function

def send user
# if it's Monday send email with all events
# else send email with only the updated events
puts "sending email to #{user.name}"
end
end

user = User.new('josh')
party = Event.new('party', 'http://events.com/events/123', 1234234)
movie = Event.new('movie', 'http://events/events/564', 1243133)
user.events = [party, movie]

user.get_time

if user.updated? or Date.now.monday?
Email.send(user)
end

 

[Robert Klemme]

I would like to get your feedback on the initial design of my app.
The goal is to email a user when an event he cares about change it's time.
i have User class, Event Class and an Email module.

some questions:
1. should Email be a class instead of a module?
2. should Email be refactored into a method of the User class?

With the current logic a module or class of its own seems overkill.
However, if you need to do formatting etc. to send out an email a
separate class might be appropriate. I'd probably construct an instance
of Email for every User that is to receive an email, set all necessary
properties (address, changed events) and then have a single method for
sending (btw. #send might not be a good name because of Object#send).

3. I read a bit about DCI Design Pattern (http://www.clean-ruby.com/ and http://saturnflyer.com/blog/jim/201...what-your-system-is-vs-what-your-system-does/) and I am curious if there is a way to use it here.
In DCI I believe I'll need a context module that will describe the main use cases of my app,
and it each use case i'll add roles to some objects. in my case the user object will be extended with Email module (the Email will turn into a role, being able to send emails).

Frankly, I believe your application - which really seems to be a single
use case only - is not complex enough to think about how to bring DCI
into this. I have to admit, I just glanced over the text at
http://www.artima.com/articles/dci_vision.html because I did not find it
well structured, but the main point seems to be that functionality is
not attached to individual classes but rather use cases and multiple
classes. I believe this is what is called "multiple dispatch" elsewhere
(Common Lisp has it IIRC).

I pasted my code below but it's nicer here: http://pastebin.com/XA6ez8FY

There's one thing I'd change about your code: change @updated in Event
from a boolean flag to the point in Time of last update (or throw it
away since @base_time might have the proper semantics already) and that
in User to point in time of last check. This has a few advantages

- The email sending is a read only operation for Events (the flag does
not need to be changed any more) and modification is restricted to
methods which actually modify an Event.
- If there are multiple clients interested in checking whether an Event
has changed the code will break with a flag only (for example, you might
want to send an administrator a single email per day with all changed
events for monitoring or debugging purposes)
- You can devise other action schemes (e.g. order Events in the email by
modification time).

Also, get rid of @current_time - this is what Time.now is for. This
again makes querying a read only operation while currently you need to
update @current_time.

Kind regards

robert

 

[josh]

Thanks for your insights Robert.

Here is another question - should I pass the user object as argument
of method that belong to email object or is it better to pass an email
object into a method that belong to the user object?

 

[Robert Klemme]

Thanks for your insights Robert.

Here is another question - should I pass the user object as argument
of method that belong to email object or is it better to pass an email
object into a method that belong to the user object?

I'd probably do

class User
def send_updates(after = @updated)
evts = @evetns.select {|e| e.updated >= after}

unless evts.empty?
em = Email.new
em.user = self
em.events = evts
em.whatever_else = 123
em.send_mail
@updated = Time.now
end
end
end

You could as well also do this externally from User, e.g.

class UpdateEmail
attr_accessor :user, :events

def self.send_updates(user)
em = new
em.user = user
em.events = user.updated_events
em.whatever_else = 123
em.send_mail
end
end

UpdateEmail.send_updates a_user

Kind regards

robert

 

[josh]

nice!

I think i like the second option better.
maybe I feel that the user should not be responsible for sending
emails.
I am curious if there is any advantage regarding testing it.
I believe that in both cases I would stub the Email.send_email method.

also, is it ok to send an email object to User.send_updates (the first
option) instead of creating the object inside the method?
not sure which one is considered better. it's just a few thoughts I
always have when designing an app.

 

[Robert Klemme]

nice!

Thank you!

I think i like the second option better.
maybe I feel that the user should not be responsible for sending
emails.

I tend to prefer the first option - probably because the instance of
Email needs so much information that User does not necessarily have to
exhibit publicly. I'd probably have to muse about it a bit more.

Btw., I'd do slight changes:

class User
def send_updates(after = @updated)
evts = @events.select {|e| e.updated >= after}
now = Time.now # reduce risk of race conditions

unless evts.empty?
em = Email.new
em.user = self # or set specific fields like mail_addr
em.events = evts
em.whatever_else = 123

em.send_mail
end

# only set if sending was successful:
@updated = now

not evts.empty?
end
end

I am curious if there is any advantage regarding testing it.
I believe that in both cases I would stub the Email.send_email method.

For testing you could override the method in Email that does delivery.
Or you have an instance of a separate class for email delivery which
then can be mocked.

also, is it ok to send an email object to User.send_updates (the first
option) instead of creating the object inside the method?

Yes, you could reuse an Email instance. But that is only worthwhile
IMHO if the cost of creating an Email instance is significant. If not
I'd keep it an implementation detail of User so users of User do not
need to know.

not sure which one is considered better. it's just a few thoughts I
always have when designing an app.

Yep, and these change over time together with requirements and
experience - so there is no all time best solution. But there aren't
silver bullets either...

Kind regards

robert