Water - Marj & Sophia #26
Conversation
There was a problem hiding this comment.
Slack CLI
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
Practices best practices working with APIs. The .env is not checked into git, and no API token was directly used in the Ruby code without ENV. |
✔️ |
| Practices error handling with APIs. For all pieces of code that make an API call, it handles API requests that come back with errors/error status codes appropriately. | In the send_message method, you return the status code essentially. However, it might be good to check in Channel#list_all and User#list_all to have behavior in case the self.get returns unsuccessfully. Your code actually does break if self.get returned unsuccessfully! It would also be nice to start anticipating failures like this in the tests for those methods, too! |
Implements inheritance and inheritance idioms. There is a Recipient class. User and Channel inherit from Recipient. In Recipient, there are appropriate methods defined that are used in both User and Channel. Some may be implemented. Some may be template methods. |
✔️ |
Practices clean code. lib/slack.rb only interacts with Workspace to show a separation of responsibilities. Complex logic is broken into smaller helper methods. |
✔️ |
| Practices instance methods vs. class methods appropriately. The methods to list all Channels or Users is likely a class method within those respective classes. | ✔️ |
| Practices best practices for testing. The project has and uses VCR mocking when running tests, and can run offline. | ✔️ |
Practices writing tests. The User, Channel, and Workspace classes have unit tests. |
✔️ |
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in Recipient) |
There's one failing test not quite up to standard :( |
| Practices git with at least 15 small commits and meaningful commit messages | ✔️ |
Functional Requirements
| Functional Requirement | yes/no |
|---|---|
| As a user of the CLI program, I can list users and channels | ✔️ |
| As a user of the CLI program, I can select users and channels | ✔️ |
| As a user of the CLI program, I can show the details of a selected user or channel | ✔️ |
| As a user of the CLI program, when I input something inappropriately, the program runs without crashing | ✔️ |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 7+ in Code Review && 3+ in Functional Requirements | ✔️ |
| Yellow (Approaches Standards) | 6+ in Code Review && 2+ in Functional Requirements | |
| Red (Not at Standard) | 0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging |
Additional Feedback
Hi Marj and Sophia!! Great work on this project!!
Overall, your code is GREAT! The code generally has great style; you all wrote and tested the classes very well, and you have VCR and testing working. Your program works exactly as expected.
There are a few major pieces of feedback I have for you:
- Most importantly, the tests for
list_allwere a little strange-- you all usedWorkspace.newas the "Act" step for thelist_allmethod. In general, we'd want thelist_alltests to be able to directly callRecipient.list_allorChannel.list_allorUser.list_all, and not an implied call to that withWorkspace.new. Let me know if you have questions-- this is a tough concept and I want to help get it better! - You all didn't get a chance to finish writing tests for
send_message-- the current test you have fails, and I think there's room for at least one more test onsend_messagethat expect a failure
Other than that, I really only have minor nitpicking comments to help suggest refactoring and code style and things to think about. These are all very minor-- I share them for you all to think about while writing code in the future. :)
Let me know if there are any questions! Great work on this project, well done!
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | ✅ |
| Elegant/Clever | |
| Descriptive/Readable | |
| Concise | ✅ |
| Logical/Organized |
| user_input = 'slackbot' | ||
| message = 'test message' | ||
| @selected = @workspace.select_user(user_input) | ||
| expect(@workspace.users[0].send_message(message)).must_equal false |
There was a problem hiding this comment.
This test ends up failing...
Your test here expects a successful send_message to return false! In practice, your successful send_message ends up returning true!
This test failure is a little weird to me-- it'd probably be better to expect true here, or make it clear in your implementation why your test is failing and why it should return false
| def details | ||
| raise NotImplementedError, 'Implement me in a child class!' | ||
| end | ||
|
|
||
| def self.list_all | ||
| raise NotImplementedError, 'Implement me in a child class!' | ||
| end |
There was a problem hiding this comment.
Well done on making these template methods!!
| puts "Enter your message here:" | ||
| message = gets.chomp | ||
| http_post_response = @selected.send_message(message) | ||
| p http_post_response |
There was a problem hiding this comment.
You probably don't need this line for the project submission!
| http_post_response = @selected.send_message(message) | ||
| p http_post_response |
There was a problem hiding this comment.
Here, you name the variable http_post_response. I agree that during @selected.send_message method, a POST request is sent at one point. However, what does @selected.send_message return? It doesn't return an http POST response! It returns true or false based on the current implementation
It might be good to reconsider the variable name here. I might put is_successful_message or is_successful
| p http_post_response | ||
| end | ||
|
|
||
| return !!http_post_response |
There was a problem hiding this comment.
!! on a boolean is just the original boolean!
AKA
!true is false, and !!true is true
!false is true, and !!false is false
| describe "Recipient.list_all method call" do | ||
| it "should return an array of channel instances" do | ||
| expect(@workspace.channels).must_be_kind_of Array | ||
| expect(@workspace.channels.length).must_equal 3 | ||
| end |
There was a problem hiding this comment.
Technically waht we want when we test the Recipient#list_all method is to most directly test the method... AKA call it directly if we can!
In this case, that means getting a Recipient and calling .list_all on it. Because you have a template thing going on with list_all, actually, one of the best options you have is probably to make a test in Channel and in User to test list_all for both classes.... and to also test that Recipient.list_all gives back a NotImplementedError!
Again, the act step of the test should be something like Channel.list_all. I know that gets called during Workspace.new, but when we unit test, we want to get more direct.
| end | ||
|
|
||
| def select_channel(user_input) | ||
| user_input |
There was a problem hiding this comment.
This line technically doesn't do anything! Feel free to delete it
| results = selected.nil? ? "This is not a valid Channel" : selected.name | ||
| puts "Result: #{results} " |
There was a problem hiding this comment.
Following some other comments-- it might be interesting to move these lines back into slack.rb. You're returning selected anyway back to slack.rb, so you can do this print logic there too!
| end | ||
|
|
||
| def select_user(user_input) | ||
| user_input |
There was a problem hiding this comment.
Similar to above-- this line of code doesn't do anything! Feel free to delete it
| if user_input | ||
| selected = @users.find { |hash| hash.name == user_input } | ||
|
|
||
| if selected.nil? | ||
| selected = @users.find {|hash| hash.slack_id == user_input } | ||
| end | ||
|
|
||
| results = selected.nil? ? "This is not a valid User" : selected.name | ||
| puts "Result: #{results} " | ||
| end |
There was a problem hiding this comment.
Looking at this nested if selected.nil?... I feel like there's a chance for refactoring here to make these ifs slimmer and cooler. Honestly, I don't think this is necessary, and I haven't actually thought of a suggestion to refactor 😅 , but I thought it was fun that I'm inspired by your code to look at it more deeply. That's a sign of well-written code!
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection