Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Joseph Bloom #603

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Joseph Bloom #603

wants to merge 43 commits into from

Conversation

Jsphbloom
Copy link

No description provided.

Jsphbloom and others added 30 commits December 12, 2024 13:24
git push

git status
require './lib/dmv_data_service'


class DmvFacilities

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make more sense to call this class DMVFactory to match the Vehicle Factory naming convention

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree ^


def administer_written_test(registrant)
if @services.include?("Written Test") && registrant.permit? == true
registrant.license_data[:written] = true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely works, but it would be a little better if there was a method on the registrant class that managed this change. It could either be explicitly exposing license_data as an attr_writer or attr_accessor, or you could create a method to earn_written_license that would change the value from inside the registrant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! i thought it was a little weird that I was handling this from facility.

@address = address
@phone = phone
def initialize(facility_details)
@name = facility_details[:name] || facility_details[:dmv_office] || facility_details[:office_name]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of this OR logic 😉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you B)

describe '#register vehicle' do
it 'can register a vehicle' do
@facility.register_vehicle(@cruz)
expect(@facility.registered_vehicles).to eq([@cruz])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also want to make sure that the registration date is set when this method is called!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes gotcha! Missed that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants