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

Feedback: Changes and ideas #3

Closed
ghost opened this issue Nov 12, 2021 · 7 comments
Closed

Feedback: Changes and ideas #3

ghost opened this issue Nov 12, 2021 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Nov 12, 2021

Hi there! Very interesting project, and I've looked through some things.

There are a few things that I find quite confusing and an odd choice to add, so I've added some things I believe you could change to improve this API.

First, on the characters.
The JSON returned is
{ "name": "monika", "age": 18, "born_date": "September 22nd", "concept_height": "160 cm", "gender": "Female", "hair_color": "Coral Brown", "eye_color": "Emerald Green", "filename": "monika.chr", "appears": ["Act 1", "Act 2", "Act 3", "Act 4"], "voice_actor": "Jillian Ashcraft" }
It works, but there are a few things I'd change.
First, the name should be Capitalized, so it returns Monika instead of monika.
The age is alright, they're all 18.
born_date, while personal choice of a name, i would rather change that to birthdate, as that is usually most commonly used to refer to the day someone was born.

However, I noticed you added these for the three other characters, and I believe you've been misled, as none of these characters have actual official dates. So for Yuri, Sayori and Natsuki, these should be removed to not mislead people. Only Monika's is known so far.

Concept height is good, but I'd change from string to integer for easier usability

Hair color and eye color is fine imho, but I would suggest using a hex code, but as they are, is fine.

Voice actor is cool.

I noticed your "no character found" error response, which I believe could be swapped out for simply sending back a 400 HTTP Bad Request code instead.

On poems, they're alright, but I noticed you have a translation variable. No idea if you planned to fix this later, but I'd change how these work if thats the case. Maybe add an alternative request handle if you want a specific translation of a poem in a specific language.

The only thing that sticks out to me is the occasion field. Its a bit clunky, but it works. I'd change from string to integer to indicate which day instead.

A suggestion I propose is a font field, where you can specify the Doki's font, if that is useful to someone.

Have a nice day!

@ghost
Copy link
Author

ghost commented Nov 12, 2021

Closing because we've discussed in Discord DMs.

@ghost ghost closed this as completed Nov 12, 2021
UltiRequiem added a commit that referenced this issue Nov 12, 2021
UltiRequiem added a commit that referenced this issue Nov 12, 2021
@UltiRequiem UltiRequiem self-assigned this Nov 12, 2021
@UltiRequiem UltiRequiem added the enhancement New feature or request label Nov 12, 2021
@UltiRequiem UltiRequiem pinned this issue Nov 12, 2021
@UltiRequiem
Copy link
Owner

UltiRequiem commented Nov 12, 2021

First, the name should be Capitalized, so it returns Monika instead of monika.

Done:

fetch('https://ddlcapi.herokuapp.com/characters/monika')
.then(response => response.json())
.then(json => console.log(json.name)) // Monika

@UltiRequiem
Copy link
Owner

born_date, while personal choice of a name, i would rather change that to birthdate, as that is usually most commonly used to refer to the day someone was born.

Done:

fetch('https://ddlcapi.herokuapp.com/characters/monika')
.then(response => response.json())
.then(json => console.log(json.born_date)) // undefined
 fetch('https://ddlcapi.herokuapp.com/characters/monika')
.then(response => response.json())
.then(json => console.log(json.birthday)) // September 22nd

@UltiRequiem
Copy link
Owner

Concept height is good, but I'd change from string to integer for easier usability

Now return something like:

"concept_height": {
  "cm": 160,
  "inches": "5'3"
}

I think its okay, what do you think?

@UltiRequiem
Copy link
Owner

UltiRequiem commented Nov 12, 2021

I noticed your "no character found" error response, which I believe could be swapped out for simply sending back a 400 HTTP Bad Request code instead.

Appart from the no character found error response, the status code is 404, I think that is ok.

UltiRequiem added a commit that referenced this issue Nov 12, 2021
@UltiRequiem
Copy link
Owner

The only thing that sticks out to me is the occasion field. Its a bit clunky, but it works. I'd change from string to integer to indicate which day instead.

I decided to leave the occasion field and add your suggestion to the day property.

@ghost
Copy link
Author

ghost commented Nov 12, 2021

Great! Looks good now

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant