Mark Kendall #602
Mark Kendall #602
Conversation
Fix existing codebase - merge with main
Registrant class and rspec files - merge with main
…ed variables in Vehicle class
Feature facility services - merge with main
…r, and associated test
…pular make/model working correct
…rations in year working
… new functionality
…and tests necessary for new functionality
…d hours and holidays access / functionality
… NY vehicles - working
…e NY vehicle data
…lity Feature analytics and functionality - merge with main
…lity Add helper methods to separate dataset formatting for each state - merge with main
| #NOTE: I kept method naming based on EV vehicles. However, now that I've extended functionality to allow NY vehicles (later in iteration 4), this method is more general, | ||
| #and can analyze general NY vehicles. | ||
|
|
||
| vehicle_tally = {} |
There was a problem hiding this comment.
Consider breaking up some of this logic into well-named helper methods in order to shorten this method. We typically want methods to stay between 8-10 lines, so using a helper method like tally_vehicle could be a great way to extract some of this into a separate method.
| end | ||
| end | ||
|
|
||
| #Find county which has the most vehicle registrations. Note high machinery overlap with most popular model...a way to combine? |
There was a problem hiding this comment.
I love seeing your note around combining! Extracting a helper method that just uses a different keyword value to look for, and updates the correct key in the hash, could do the trick!
| #Hours are listed for each day by opening and closing. For now, I'm going to brute-force this, | ||
| #since no exact format is expected. This is a mess though: | ||
| #This is gross...some facilities don't have keys for all M-F. So I need to run ifs now (otherwise problems with nil) | ||
| hours_formatted = "" |
There was a problem hiding this comment.
This data cleaning does get a bit icky. That's why extracting into a helper method that can be called such as format_opening_times could at least encapsulate the ickyness.
| def renew_drivers_license(registrant) | ||
| registrant.license_data[:renewed] = include?("Renew License") && registrant.license_data[:license] == true | ||
| end | ||
|
|
| :engine | ||
| :engine, | ||
| :registration_county | ||
| attr_accessor :registration_date, :plate_type |
There was a problem hiding this comment.
For the purposes of this project, it is totally okay to use an attr_accessor. However, we often want to stay away from these and instead create methods for changing the value of our attributes. We want to be able to have control over these data changes, and maintain data integrity. Right now, someone could change the registration_date to banana and we have no control over that. But, if you added a "setter" method in this class that an outside class had to call, like set_registration_date(date) then you could add logic like checking that it's a valid date. This is a good rule of thumb for adhering to the principle of encapsulation in our our classes.
|
|
||
| # binding.pry | ||
|
|
||
| #NOTE: this dataset list actually changed on me after I made tests; hence why I've moved to more 'permanent' testable aspects |
There was a problem hiding this comment.
Great observation. We'll talk more about this when you get to Mod 3, but when we're using external data sources we can either:
- Make somewhat flexible assertions or only assert on the permanent aspects OR
- Use some mocked data instead of calling the DMVDataService. You could, for example, create a hash in this test that includes 3 facilities as they appear in the MO dataset, and that is called a "stub". You can then test for exact equivalence of different attributes once the Facility objects are created. Your "fake" facilities list hash would replace line 86, and it would be that stubbed data you pass into the create_state_facilities method
I'd argue your tests here are just fine, but sometimes we want to be able to make more robust assertions so it's helpful to use stubbed data and control exactly what those attributes will be
|
|
||
| max_upgrades_hash = {written: true, license: true, renewed: true} | ||
| expect(@registrant_1.license_data).to eq(max_upgrades_hash) | ||
| end |
There was a problem hiding this comment.
Excellent robust testing!
Project is complete, iterations 1-4 implemented (option #3 chosen for iteration 4).
All RSpec test files working, tests pass as of submission.