5
\$\begingroup\$

For a homework assignment for an online class (pretty much learning everything from the internet anyway) we were asked to build an appointment program. This is just a start, however I feel I am definitely over-complicating things and going about this the complete wrong way.

These were the instructions we were given:

Implement a superclass Appointment and sub classes OneTime, Day and Month. An appointment has a description (for example, "Root Canal"), and dates information (you can use Date object or Int Year, Int Month, Int Day). Fill an array of Appointment objects with a mixture of appointments.

Write a method OccursOn inside each of the sub classes that checks whether the appointment occurs on that date (OneTime), day (Day) or month (Month). Ask the user to enter a date to check (for example, 2006 10 5), and ask to user if they want to check against OneTime, Day or Month appointment. Based on what the user selected, OccursOn inside each of the sub class should run and display any matching appointment and associated descriptions.

require 'date'

class Appointment
    attr_accessor :info, :date, :year, :day, :month
    def initialize(info, date)
        @info = info
        @date = Date.parse(date)
    end

    def get_info
        @info
    end

    def get_date
        @date
    end

    def get_year
        @year = date.year
    end

    def get_month
        @month = date.mon
    end

end

class OneTime < Appointment

    def OccursOn(year, month, day)
        puts "The year you entered is #{year}"
        puts "The month you entered is #{month}"
        puts "The day you entered is #{day}"
        appointments.each do |x|
            puts "Month #{x.get_month}"
            if x.get_month == month
                puts x.get_date
                puts x.get_info
            end
        end
    end

end

class Day < Appointment

    def OccursOn(day)
        puts day
    end

end

class Month < Appointment

    def OccursOn(month)
        puts month
    end
end

appointments = []
appointment1 = Appointment.new("Root Canal", "31/10/2017")
appointment2 = Appointment.new("Cavity Filling", "31/12/2017")
appointments << appointment1 << appointment2
puts appointment1.get_date
puts appointment2.get_date
puts appointment1.get_info

onetime1 = OneTime.new("Root Canal", "31-10-2017")

puts "Enter a date to check (for example, 31-10-2017)"
checkdate = gets.chomp
checkdate = Date.parse(checkdate)

puts "Would you like to check against OneTime, Day or Month appointment?"
checkagainst = gets.chomp

if checkagainst == "OneTime" or checkagainst == "onetime"
    onetime1.OccursOn(checkdate.year, checkdate.month, checkdate.day)
elsif checkagainst == "Day" or checkagainst == "day"
    puts "Day" # run day check function 
elsif checkagainst == "Month" or checkagainst == "Month"
    puts "Month" # run month check function
else
    puts "Invalid option."
end
\$\endgroup\$
5
+50
\$\begingroup\$

I feel I am definitely overcomplicating things

I agree with you that the code is longer than necessary. Let's take a look.

YAGNI - you ain't gonna need it

Try to avoid writing code until a caller (such as a test method for a Story) forces you to write it. Then you know you really need it.

attr_accessor :info, :date, :year, :day, :month

The first two look good. I didn't notice any compelling reason to list y,m,d, as caller could get those from date. The day might be called mday for consistency with library.

def get_info
    @info
end

def get_date
    @date
end

Ummm, didn't attr_accessor already take care of those? We're writing in ruby, not java. Take advantage of its strengths!

I commented above that get_year & get_month seem redundant. Also, it seems odd to list them without get_mday (or perhaps get_day). Better to elide them entirely.

    appointments.each do |x|

This seems to be a global reference. I can't imagine that's a good thing. Consider moving that list into a class, such as the Appointment class.

        if x.get_month == month

I suggest it would be more natural to phrase that as x.date.mon.

Your recurring Day & Month classes appear to be unimplemented stubs, so far. You have a design decision to make. Will adding a recurring appointment create several new entries until a date "very far" in the future? Or will it add a single entry and then you deal with that at query time? There is no right answer; each offers its own trade-offs.

puts "Would you like to check against OneTime, Day or Month appointment?"
checkagainst = gets.chomp

This seems a cumbersome UX. Rather than prompting, simply loop over all three, reporting the appointment and its type whenever you find a match.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback! How would you recommend putting the appointments array into the class to avoid the global reference? \$\endgroup\$ – tda Oct 8 '17 at 21:39
  • \$\begingroup\$ It seems likely that you want a container class, such as PersonalCalendar, which can add various sorts of appointments in a conflict-free way, and also respond to date queries. That would be a natural place to put it. \$\endgroup\$ – J_H Oct 9 '17 at 4:24
  • \$\begingroup\$ Unfortunately I am not certain on how to complete that. \$\endgroup\$ – tda Oct 10 '17 at 20:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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