Skip to content

Sockets - Chantal & Kate#24

Open
ChantalDemissie wants to merge 15 commits intoAda-C11:masterfrom
ChantalDemissie:master
Open

Sockets - Chantal & Kate#24
ChantalDemissie wants to merge 15 commits intoAda-C11:masterfrom
ChantalDemissie:master

Conversation

@ChantalDemissie
Copy link

Question Answer
How did you go about exploring the Slack API? Did you learn anything that would be useful for your next project involving an API? We explored the Slack API documentation by reviewing it and discussing it. We also learned alot from the testing feature on https://api.slack.com/methods/channels.list/test
Give a short summary of the request/response cycle. Where does your program fit into that scheme? Its when a client sends a request message to a server, and then the server handles the request and sends a response back. We used the API across the program. It is pulling the data for our program.
How does your program check for and handle errors when using the Slack API? Errors and checked for and handled with VCR and tests.
Did you need to make any changes to the design work we did in class? If so, what were they? Yes, we used a slack cli folder, and adapted our original design to the provided template.
Did you use any of the inheritance idioms we've talked about in class? How?  
How does VCR aid in testing a program that uses an API? VCR makes the program more cost effective because it stores all the old tests and only uses the API on new ones.

@droberts-sea
Copy link

slack.rb

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene (no slack tokens) yes - this commit history is looking a lot better! I would like to have seen commits from both partners, even if you're pairing on one computer it's good to switch off, if only to practice the push/pull/merge process.
Comprehension questions yes
Functionality
List users/channels yes
Select user/channel yes
Show details no
Send message no
Program runs without crashing yes
Implementation
API errors are handled appropriately some - you check for an error and raise an exception, but the user interface doesn't handle that exception gracefully and it still results in a crash.
Inheritance model matches in-class activity looks like you were just getting started on this
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately N/A
Methods are used to break work down into simpler tasks yes
Class and instance methods are used appropriately yes
Overall

This is a good start. The code that you've written so far is solid, and I suspect you're not too far from a complete implementation. It's clear to me that you have a handle on the request/response cycle and consuming an API.

There are two important learning goals that I don't see being met completely by this code:

  • Testing, especially coming up with edge and failure cases
  • Implementing a design using inheritance from scratch

These are difficult things to do, and it's natural to be struggling with them at this point. I do see your Ruby fundamentals coming together, so I am not overly concerned - it's just something to be aware of and keep an eye on. Continue to reach out for help when you need it, and keep up the hard work!

@commands = {
'list users' => self.method(:list_users),
'list channels' => self.method(:list_channels),
'select user' => self.method(:select_user),

Choose a reason for hiding this comment

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

This is a pretty advanced technique for organizing these method calls. It works, and is arguably much cleaner than a big switch statement, but it's a little concerning that you spent time figuring out how to do this but didn't complete all the waves.

In general, it is wise to do it the way you know first, then refactor it to the fancy way if you have time later. That's especially true if it's something not connected to the learning goals of the assignment.

If this code or the idea for it came from a tutor, please pass along this feedback!

def select_channel(selected_channel)
channels.each do |channel|
if channel.name == selected_channel || channel.id == selected_channel
@selected = channel

Choose a reason for hiding this comment

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

Ruby's .find enumerable method might be helpful to clarify this code.

def channels
response = self.class.get('/channels.list', @options).parsed_response
raise 'Failed to get channels' unless response['ok']

Choose a reason for hiding this comment

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

When you raise an exception you should provide an exception type! Otherwise you get a RuntimeError, which is pretty generic. Defining a custom exception type like SlackApiError might be a good choice here.

def users
response = self.class.get('/users.list', @options).parsed_response
raise 'Failed to get users' unless response['ok']

Choose a reason for hiding this comment

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

For this method (and channels above), I have two comments:

  • Since there's a lot of code here that has to do with creating users, this code might fit well as a class method on User
  • Since API calls are expensive, it might make sense to do this work once in the constructor, and save the results in instance variables

The end result might look like this:

# user.rb
class User
  def self.list
    # move the code currently in Workspace#users to here
  end
  # ...
end

# workspace.rb
class Workspace
  attr_reader :channels, :users
  def initialize
    @channels = Channel.list
    @users = User.list
    # ...
  end
  # ...
end

describe "select_user" do
it "finds a user by username" do
VCR.use_cassette("select user") do
# arrange

Choose a reason for hiding this comment

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

Missing test cases for this (and select_channel above):

  • User does not exist
  • What happens to a previously selected user if you try to select a user that D.N.E.?

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