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

Reservation #2

Merged
merged 22 commits into from
Apr 6, 2024
Merged

Reservation #2

merged 22 commits into from
Apr 6, 2024

Conversation

KaiHog
Copy link
Collaborator

@KaiHog KaiHog commented Apr 1, 2024

No description provided.

Copy link
Collaborator

@Stefhanos Stefhanos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik vind dat je het goed gedaan hebt, voor zover ik kan zien zit alles uit Trello erin verwerkt, en ben je niks vergeten. IK heb het nog niet functioneel getest maar qua code ziet het er in orde uit en zie ik geen bugs. Kijk nog wel even naar mijn 2 comments over de naam van de class en over de documentatie. Sommige dingen zijn duidelijker dan anderen en niet alles hoeft comments te hebben. Netjes gedaan!

ProjectB/Tour.cs Outdated
public string Location { get; set; } // Location of the tour
public int Capacity { get; set; } // Maximum capacity of the tour
public int ParticipantsCount { get; set; } // Number of participants signed up for the tour

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deze comments voor de aangemaakte fields zijn niet nodig, aangezien de namen van de fields al duidelijk genoeg aangeven waarvoor het is

using System.Linq;
using System.Text.Json;

class Program
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik denk dat het beter is als deze class Reservation heet en niet program

Copy link
Owner

@marcus-talbot42 marcus-talbot42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nog wat dingen die eventueel anders kunnen. Als je daar hulp bij wil, kunnen we er even doorheen lopen.

ProjectB/Participant.cs Outdated Show resolved Hide resolved
ProjectB/Participant.cs Outdated Show resolved Hide resolved
ProjectB/Tour.cs Outdated Show resolved Hide resolved
@KaiHog KaiHog closed this Apr 5, 2024
Copy link
Owner

@marcus-talbot42 marcus-talbot42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vergeet niet om testen te schrijven.


/// <summary>
/// Blueprint for users, containing the fields shared by all user types. This class should be implemented by sub-classes,
/// which specify the type of user, and implement unique functionality and fields based on that type.
/// </summary>
/// <param name="username">The username assigned to the user.</param>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er zijn een aantal van mijn comments weggevallen tijdens je commits. Zou je die weer terug willen plaatsen?


/// <summary>
/// Blueprint for users, containing the fields shared by all user types. This class should be implemented by sub-classes,
/// which specify the type of user, and implement unique functionality and fields based on that type.
/// </summary>
/// <param name="username">The username assigned to the user.</param>
/// <param name="role">The role assigned to a user, used to determine what functionality should be available to said user.</param>
public abstract class AbstractUser(string username, UserRole role) : IEquatable<AbstractUser>, IEntity<string>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deze wijziging is niet nodig.

[JsonProperty] protected readonly string Username;
[JsonProperty] protected readonly UserRole Role;

public AbstractUser(string username, UserRole role)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aparte constructor is niet nodig, omdat we die effectief al definiëren direct naar de naam van de class.

@@ -65,9 +68,10 @@ public override int GetHashCode()
private class Builder
{
private string? _username;
private string? _password;
private int _ticketNumber; // Add ticketNumber field
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Een apart ticket-number field is niet nodig. We kunnen de _username gewoon gebruiken.

/// UserRole.Guest, the password will be ignored.
/// </summary>
/// <param name="password">The password that will be set.</param>
/// <returns>This instance of the Builder.</returns>
public Builder WithPassword(string password)
{
_password = BCrypt.EnhancedHashPassword(password);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gelieve wel weer EnhancedHashPassword gebruiken. Anders werkt de verification ook niet meer.

public bool IsValid() => _validForDate.CompareTo(DateTime.Today) == 0;

public override bool Equals(AbstractUser? other)
public class Guest : AbstractUser
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Het wijzigen van de constructor is niet nodig, omdat ticketnumber geen nieuwe field hoeft te zijn. Gebruik username daarvoor.


public class JsonFileReader<T>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We hebben al een class ProjectB.IO.JsonFileReader. Die kun je gebruiken, zodat we niet dit soort code door het hele project hebben slingeren.


public RondleidingService(JsonFileReader jsonFileReader)
public RondleidingService(JsonFileReader<Tour> jsonFileReader) // Adjust the constructor parameter
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In plaats van de data pas in te lezen wanneer je een rondleiding wil laten zien, kun je dat eigenlijk al direct doen wanneer het RondleidingService-object gemaakt wordt. Zou je in de constructor kunnen doen.

for (int i = 0; i < amount; i++)
{
// Generate a unique ticket number for each guest
int ticketNumber = i + 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticketNumber is hier ook niet meer nodig.

@marcus-talbot42 marcus-talbot42 merged commit 5ff3c7e into main Apr 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants