Nikey646 / VndbSharp

A C# Vndb API Library. #OriginalNamingScheme
MIT License
18 stars 4 forks source link

Created Set Wishlist and Set VisualNovelList #6

Closed micah686 closed 7 years ago

micah686 commented 7 years ago

I know the SetVisualNovelListAsync is bad, but it should at least allow running the command for now. I would like to find a better way of doing it, because it's a little large for the API.

Nikey646 commented 7 years ago

This was the best i could come up with the reduce the amount of repeated code. The way it works is that if a status is null, then that means you want to remove the entry. But if it's not, then you are just updating the entry. In this case, because notes accepts a null value as a legitimate use, we don't actually need to check it. We might want to provide an overload for just updating the status or notes with similar conditions, so they don't need to send the notes every request.

This does use a modified version of the SendRequestInternalAsync(String, UInt32, Object) method that i'll commit into the repo once i fix the last little bit (currently if both status and notes are null, it literally appends "null")

public async Task<Boolean> SetVisualNiovelListAsync(UInt32 id, Status? status, String notes)
    => await this.SendRequestInternalAsync(Constants.SetVisualNovelListCommand, id, status.HasValue ? new { status, notes } : null, true).ConfigureAwait(false);
micah686 commented 7 years ago

I've also recently moved the code for the Database dumps to a new class, like your notes/TODO's said to do.

Nikey646 commented 7 years ago

I forgot to mention when i did the review, the code as-is, is perfectly fine. The comments were mostly remarks, and anything that would cause problems with my future plans (Like the class not being static) i can easily change later. I've not merged the PR due to waiting on my aforementioned question being answered by yorhel.

micah686 commented 7 years ago

Yorhel's response:

Regarding downloading dumps: The server's fine on bandwidth, so you don't have to worry much about that. But if it's easy to cache it may still be worth doing so to optimize your users' bandwidth. :) Regarding client name+version: Preferably, let the application set these. https://vndb.org/t3599/7#162

I figure it's easier checking vndb now and then for a response, because vndb doesn't have email notifications.