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

Feature/auto bid #15

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

Feature/auto bid #15

wants to merge 2 commits into from

Conversation

serpient
Copy link
Member

@serpient serpient commented Aug 8, 2019

Things Done

  • Add max bid price to Bid schema
  • Add automatic bidding feature

- Update Bid Schema
- Add validation for incoming BId max_bid_price, to ensure that if used, it is not equal or lower than price (the minimum price).
- Update returned bid json to include a max_bid_price
After creating a new bid, bid controller `handle_auto_bids` will take the item id, grab all current active auto bids for the item, then iterate through each and create new bids for that user if the new price increment is less than the users maximum bid price. It will continue this recursive check until there is one auto bid left (aka, there is no more competing auto bid requests).  It also makes sure that a user doesn't double bid accidentally.
defp handle_auto_bids(item_id) do
item_id
|> UserItem.get_auto_bids_by_item
|> run_auto_bids(item_id)
Copy link
Member

@tpetersen0308 tpetersen0308 Aug 9, 2019

Choose a reason for hiding this comment

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

This may be nitpicky, but I think it makes for better readability to implement pipelines when the same message is being passed through. So passing in item_id to a function that returns a collection of bids that get passed to run_auto_bids doesn't read as well to me as something more like UserItem.get_auto_bids_by_item(item_id) |> run_auto_bids(item_id). Such a minor thing here, but I think it becomes more helpful with more complicated pipelines.

|> UserItem.get_auto_bids_by_item
|> run_auto_bids(item_id)

case length(item_id |> UserItem.get_auto_bids_by_item) do
Copy link
Member

Choose a reason for hiding this comment

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

Could you reduce duplication and improve readability by assigning the collection of bids to a variable?


defp run_auto_bids(bids, item_id) do
bids
|> Enum.each(fn bid ->
Copy link
Member

Choose a reason for hiding this comment

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

I believe Elixir pattern-matching can be leveraged recursively to the same effect, if you want to do this more idiomatically than with the use of each.

UserItem.create_bid(new_bid)
end
end)
end
Copy link
Member

Choose a reason for hiding this comment

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

The auto-bidding functionality may be worth extracting to a separate module. The controller should really just be concerned with handling requests.

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