Badge trading#231
Conversation
| await context.ParseArgs<AnyOrder<User, PkmnSpecies, Optional<PositiveInt>, Optional<string>, Optional<bool>>>(); | ||
| int amount = amountOpt.Map(i => i.Number).OrElse(1); | ||
| int form = PkmnForms.pokemonHasForms(species) ? 0 : 1; // default to the first listed form if form is unspecified | ||
| bool shiny = shinyOpt.IsPresent ? shinyOpt.Value : false; |
There was a problem hiding this comment.
can be condensed to shinyOpt.OrElse(false)
Felk
left a comment
Felk
left a comment
There was a problem hiding this comment.
Very neat so far! You have written few TODOs, but I also don't mind merging an intermediate state as long the commands aren't hooked up yet and therefore aren't part of production code
| @@ -64,26 +68,55 @@ private void InitIndexes() | |||
| } | |||
There was a problem hiding this comment.
(above here, can't comment on the correct line).
Please add indexes for the new filterable fields.
Nitpick: Also this is fine for now, but I suspect filtering for "!= true" for the shiny field might not work properly for the index. Maybe it does, but it's something you can test if you're still motivated. E.g. by enabling profiling level 2, issuing a query, and looking at the execution plan.
| Badge.BadgeSource? source = sourceOpt.IsPresent ? sourceOpt.Value : null; | ||
| string? form = formOpt.IsPresent ? formOpt.Value.Name : null; | ||
| bool? shiny = shinyOpt.IsPresent ? shinyOpt.Value : false; | ||
| return (source, form, shiny); |
There was a problem hiding this comment.
This looks equivalent to
return (sourceOpt.OrElse(null), formOpt.OrElse(null), shinyOpt.OrElse(false));
There was a problem hiding this comment.
also is defaulting to shiny=false correct? It's still nullable, which indicates to me you wanted it to be kind of a tri-state of "yes", "no" and "don't care whether shiny or not"
| public async Task<List<BadgeBuyOffer>> FindAllBuyOffers(string? userId, PkmnSpecies species, string? form, Badge.BadgeSource? source, bool? shiny) | ||
| { | ||
| FilterDefinition<BadgeBuyOffer> filter = Builders<BadgeBuyOffer>.Filter.Empty; | ||
| if (userId != null) | ||
| filter &= Builders<BadgeBuyOffer>.Filter.Eq(b => b.UserId, userId); | ||
| if (species != null) | ||
| filter &= Builders<BadgeBuyOffer>.Filter.Eq(b => b.Species, species); | ||
| if (form != null) | ||
| filter &= Builders<BadgeBuyOffer>.Filter.Eq(b => b.Form, form); | ||
| if (source != null) | ||
| filter &= Builders<BadgeBuyOffer>.Filter.Eq(b => b.Source, source); | ||
| if (shiny == true) | ||
| filter &= Builders<BadgeBuyOffer>.Filter.Eq(b => b.Shiny, true); | ||
| else | ||
| filter &= Builders<BadgeBuyOffer>.Filter.Ne(b => b.Shiny, true); | ||
|
|
||
| return await BuyOfferCollection.Find(filter).ToListAsync(); | ||
| } |
There was a problem hiding this comment.
hmm not sure how I feel about this duplicate code. Especially since it is a bit finnicky in regards to the shiny filter requiring to filter for "!= true". If this is equal to the variant in BadgeRepo, can it be extracted as a utility method somewhere that builds a FilterDefinition<T>?
There was a problem hiding this comment.
Is something like a static method in Badge class what you had in mind?
There was a problem hiding this comment.
Something like that, yeah. Just somewhere where it's namespaced somewhat sensibly
| Badge badgeBForSale = new Badge("B", userId, species, source, after, form, shiny) { SellPrice = price }; | ||
| List<Badge> notForSale = new List<Badge> { badgeA, badgeB }; | ||
|
|
||
| _mockBadgeRepo.Setup(m => m.FindAllNotForSaleByCustom(userId, species, form, source, shiny)).Returns(Task.FromResult(notForSale)); |
There was a problem hiding this comment.
iirc there's a ReturnsAsync that removes the need to wrap the result in Task.FromResult(...)
| internal class MockClock : IClock | ||
| { | ||
| public Instant FixedCurrentInstant = Instant.FromUnixTimeSeconds(1234567890); | ||
| public Instant GetCurrentInstant() => FixedCurrentInstant; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure why I used this class somewhere in the first place. I believe it is redundant because Moq mocks should work fine
| new Transaction<User>(buyer, -1 * offer.Price, "BadgePurchase"), | ||
| new Transaction<User>(seller, offer.Price, "BadgeSale") | ||
| } | ||
| ); | ||
| await _badgeRepo.TransferBadges(new List<Badge> { badge }.ToImmutableList(), buyer.Id, "BadgeSale", new Dictionary<string, object?>() { }); |
There was a problem hiding this comment.
please put these transaction types in TransactionType
| /// Sorts badges according to the priority in which they should be sold. | ||
| /// Current sorting rule: prioritize keeping 1 of each form, then sell newer badges first. Top priority badge will be selected for sale when fufilling offers. | ||
| /// </summary> | ||
| private static List<Badge> SortBySpecialness(IEnumerable<Badge> toSort) |
There was a problem hiding this comment.
a bunch of stuff, including this, probably needs more unit tests. Maybe you'll even have to make this sorter its own thing for that to be easier
3 participants
WIP pull request for badge trading and updates to badges in general.