tawmy / xivapi-cs Goto Github PK

The repo needs at least a short readme to describe what it is (simple xivapi wrapper, not a lodestone parser), and basic usage examples.

Description

The consensus in .NET is to have names of methods dealing with asynchronous operations suffixed with Async. One such example is Stream.ReadAsync from System.IO. Doing so improves readability and provides crucial information at a glance.

Occurrences

There are 135 occurrences of this issue in the repository.

See all occurrences on DeepSource → deepsource.io/gh/Leyla-Labs/xivapi-cs/issue/CS-R1073/occurrences/

The wrapper is using models the JsonSerializer deserializes the API responses into. This would work fine if the API had consistent variable types, which it does not. For example, a Free Company weekly rank is an integer if it exists, and a string "--" if it does not.

Previously dynamic types were used to work around this issue, but they are not pleasant to use for the end user. Separate DTOs and ViewModels would help with this.

A simple separation should be enough, where the Serializer deserializes into the DTO, which is then converted into a ViewModel before it is returned. The ViewModel classes will have a constructor accepting a DTO, whose members then get converted into the ViewModels'.

Various collections are now an empty list by default instead of null since switching to ViewModels. Certain tests still check whether the value is null even though it can never be null. These need to be replaced by IsNotEmpty asserts.

A lot of test logic can probably be moved into methods that are called ten times instead of the same code being copied ten times.

New NA data center will be added in Patch 6.28:

• Dynamis

This data center has four new worlds:

• Halicarnassus
• Maduin
• Marilith
• Seraph

It's been a while since Endwalker came out. New jobs and possibly other changes need to be implemented into this repo. Until then it will not be usable.

A few helper methods have been added, and while they're public, they need to be found magically. Having their return values exposed through properties in ViewModels would make using them a lot more pleasant.

As of right now, the unit tests usually test for ten different characters. These characters are either still active, or if they're inactive, have still played the game for some time.

To make sure all values marked as non-nullable can actually never be empty, two more characters should be created in-game and added to the unit tests.

• A character that was just created and has not yet watched the first cutscene
• A character that has done nothing apart from completing the first basic tutorial so they can see other players

Once these two are added, the unit tests should fully cover all nullable properties on characters. They need to be added to all character related tests:

• CharacterProfileAchievements
• CharacterProfileExtendedComplete
• CharacterProfileFriends
• CharacterProfileMimo
• CharacterProfile
• CharacterSearch

Closes #1

The API has another property with varying return type. Free Company ranks are an integer if they exist, and a string "--" when they don't. This issue is to be resolved with #2, I'm just writing this down separately so I don't forget.

As a convenience feature, it would be great to have a method that takes a job and returns an array of melding priorities, as per Balance Discord.

AC:

• Enum of all existing jobs
• Enum of all existing meldable attributes
• Method takes one parameter, job enum
• Returns array of meldabla attributes enum
• Array is ordered, most to least important