New feature of DLC handling #51
Conversation
Added basic class for single user and class with a list of users. Added API controller to check users and their DLCs.
3Mydlo3
left a comment
3Mydlo3
left a comment
There was a problem hiding this comment.
Please look at my comments and take a quick look at Server API in Arma Server Manager.
|
|
||
| namespace ArmaForces.Boderator.BotService.DataClasses | ||
| { | ||
| public class SingleUser |
There was a problem hiding this comment.
Why not just User? This sounds awful.
|
|
||
| namespace ArmaForces.Boderator.BotService.DataClasses | ||
| { | ||
| public class Users |
There was a problem hiding this comment.
What is the purpose of this class? It doesn't have an interface. If you want some class which will allow you to retrieve users data then create an interface for it, e.g IUsersRepository and create some useful methods there instead of just returning a list of users.
ArmaForces.Boderator/ArmaForces.Boderator.BotService/DataClasses/SingleUser.cs
Show resolved
Hide resolved
| public class SingleUser | ||
| { | ||
| public ulong UserID; | ||
| public List<DLC> DLCList = new List<DLC>(); |
There was a problem hiding this comment.
This should be a property too. Don't use fields for publicly available stuff. And make both the property and the list readonly, e.g. IReadOnlyList, IReadOnlyCollection or maybe even IReadOnlySet as it should not have duplicates.
There was a problem hiding this comment.
What about adding new DLC when property is readonly?
Or you meant something like that?
private List<DLC> _dlcList;
public ReadOnlyCollection<DLC> DLCList {get { return _dlcList.AsReadOnly; }}There was a problem hiding this comment.
Yes, almost, something like this:
private List<DLC> _dlcList;
public IReadOnlyCollection<DLC> DLCList => _dlcList.AsReadOnly();We don't want to allow external modifications to any User or his data. This should be only done in IUserRepository or something like this, right where it's stored. So we need to always return immutable User object. Then to change anything, you must go through IUserRepository which will create new User instance with updated data.
| @@ -0,0 +1,26 @@ | |||
| using System.Collections.Generic; | |||
|
|
|||
| namespace ArmaForces.Boderator.BotService.DataClasses | |||
There was a problem hiding this comment.
The DataClasses is awful and stinks with legacy ArmaforcesMissionBot, don't. You are creating a DLC feature so almost everything you do should be in Features/DLC folder/namespace. This one will probably go to Models subfolder.
| //List user with given ID. "0" lists all users | ||
| [HttpGet("{userID}")] | ||
| public void UserOnServer(ulong userID) |
There was a problem hiding this comment.
No. You should have separate method for no input instead of special handling for 0 value. And method should be named GetUser. Then method for all users would just be named GetUsers or GetAllUsers.
| [Route("users/")] | ||
| [ApiController] | ||
| [Produces("application/json")] | ||
| public class DLCController : ControllerBase |
There was a problem hiding this comment.
Is it DLCController with users path? You probably should have created 2 controllers. One for users, one for dlc.
| { | ||
| [Route("users/")] | ||
| [ApiController] | ||
| [Produces("application/json")] |
There was a problem hiding this comment.
This is redundant, you can remove this.
| [Produces("application/json")] |
There was a problem hiding this comment.
Redundant with what? Something in rest of program?
There was a problem hiding this comment.
You don't need to specify what Content-Type you produce if you just use IActionResult and return the data object. It will be serialized to any Content-Type desired by caller.
| //Check if user has all DLCs | ||
| [HttpGet("DLC")] | ||
| public IActionResult UserhasDLC(ulong userID, String dlcs) | ||
| { | ||
| if (dlcs == null) | ||
| { | ||
| return (IActionResult)NotFound("No given DLC"); | ||
| } | ||
|
|
||
| List<SingleUser.DLC> dlcList = new List<SingleUser.DLC>(); | ||
|
|
||
| foreach (string singleDlc in dlcs.Split(',')) | ||
| { | ||
|
|
||
| try | ||
| { | ||
| var dlcClass = (SingleUser.DLC)Enum.Parse(typeof(SingleUser.DLC), singleDlc); | ||
| dlcList.Add(dlcClass); | ||
| } | ||
| catch | ||
| { | ||
| return (IActionResult)BadRequest("Wrong DLC name"); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| var user = _users.UsersList.SingleOrDefault(x => x.UserID == userID); | ||
|
|
||
| if (user is null) | ||
| { | ||
| return (IActionResult)NotFound("User not found"); | ||
| } | ||
|
|
||
| var dlcFound = !dlcList.Except(user.DLCList).Any(); | ||
|
|
||
| return dlcFound | ||
| ? Ok("User has all given DLCs") | ||
| : (IActionResult)NotFound("User doesn't have all given DLCs"); | ||
| } |
There was a problem hiding this comment.
This is probably useless in API. Instead just make sure API can return user and his DLCs.
Initial commit of new feature from issue #50.
Added basic class for single user and class with a list of users. Added API controller to check users and their DLCs.