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

Ports - Angela and Shamira #3

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

MiraMarshall
Copy link

slack.rb

Congratulations! You're submitting your assignment!

You and your partner should collaborate on the answers to these questions.

Comprehension Questions

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 through postman. Postman will be useful in future projects involving APIs.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? With our User and Channel classes we send a GET request to the Slack API. With our Recipient Class we use a POST request to the Slack API and include payload with headers and a body hash. The API returns a response as JSON with the requested information.
How does your program check for and handle errors when using the Slack API? We created a custom SlackApiError message that inherits from the standard error message. We raise an error if the input does not include a valid user or channel. We also raise our SlackApiError when we get a false response from the Slack API.
Did you need to make any changes to the design work we did in class? If so, what were they? Our starting classes were User, Channel, and Recipient, which was in line with our original design. After working on those classes we decided to implement a Workspace class more in line with the given design from the instructors.
Did you use any of the inheritance idioms we've talked about in class? How? The only inheritance we used was the SlackApiError that inherited from the standard error message.
How does VCR aid in testing a program that uses an API? VCR allows us to test our requests to the API with fewer calls by recording cassettes that allow us to test the code multiple times even if the API is down or we are offline.

@CheezItMan
Copy link

slack.rb

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene (no slack tokens) Good commit messages, no slack token!
Comprehension questions Check
Functionality
List users/channels Check
Select user/channel Check
Show details Check, but it doesn't label the fields returned, so a little bare bones.
Send message Check
Program runs without crashing If I enter a misspelled command the app seems to get stuck in a loop and I have to ^C to exit the app.
Implementation
API errors are handled appropriately Nope, if I select a user who doesn't exist, I get no error. Then if I send them a message the app crashes! Ditto with channel! Showing details doesn't generate an error or even a notice things went wrong.
Inheritance model matches in-class activity Nope
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately No Inheritance or polymorphism, or much composition. The design is more than a little awkward.
Methods are used to break work down into simpler tasks For the most part, see your method to send a message for a method that's not broken down well.
Class and instance methods are used appropriately You are overusing class methods here. You really should have designed User and Channel as classes that you can make instances of.
Overall You hit most of the learning goals for the project, but your design is awkward. You are not taking advantage of duck typing or polymorphism. For example if you have User or Channel instances that both have details and send_message methods you can just call them on the currently selected item and it doesn't matter if it's a channel or user. Take a look at the instructor reference design and read my inline comments. Let me know any questions you have.

require "pry"
Dotenv.load

class Channel

Choose a reason for hiding this comment

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

This class is just one class method. Instead I suggest having a Channel class with instance variables and methods. Your list method could then return an array of Channel instances.

@@ -0,0 +1,62 @@
require "httparty"

Choose a reason for hiding this comment

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

Looks like you got started on this but didn't get finished. I suggest removing it from git.


#### TODO: CHANGE VARIABLE NAME
practice = []
User.list.each do |user_name_list|

Choose a reason for hiding this comment

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

Here you end up making the same API call multiple times, this is inefficient. It might be much more useful to have list create an array of User instances with usernames, ids etc.

end
end

test_workspace = Workspace.new

Choose a reason for hiding this comment

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

Why do you create a Workspace in this Recipient class.

end

test_workspace = Workspace.new
if channel_name.include?(user)

Choose a reason for hiding this comment

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

This if-elsif block is a little awkward and could be made much cleaner with duck typing for the User and Channel classes.

class SlackApiError < StandardError; end

def select_channel(selected)
selected

Choose a reason for hiding this comment

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

What is the purpose of this method? It takes an argument and just returns the argument?

puts "4. Select Channel"
puts "5. Quit"

selection = gets.chomp

Choose a reason for hiding this comment

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

If this file isn't in your submission, please don't include it in the PR.

VCR.use_cassette("slack_channels") do
Channel.new

expect(Channel.list[0][0]).must_equal "everyone"

Choose a reason for hiding this comment

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

You can't always assume that the API will always return things in the same order. I would probably write the test to check to see if the array includes "everyone" instead.

end
end

it "will raise an error if given an empty message" do

Choose a reason for hiding this comment

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

👍


it "lists correct username" do
VCR.use_cassette("slack_users") do
User.new

Choose a reason for hiding this comment

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

Since you are using class methods you don't need to make an instance of User.

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.

3 participants