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

feat(v2): acaba logo PELO AMOR DE DEUSSSSSSSSSSSSSSSSSSSSSSS #48

Merged
merged 17 commits into from
Feb 11, 2023

Conversation

DanielHe4rt
Copy link
Contributor

No description provided.

DanielHe4rt and others added 2 commits February 10, 2023 00:32
Co-authored-by: Canhassi <80018897+Canhassi12@users.noreply.github.com>
Co-authored-by: Canhassi <80018897+Canhassi12@users.noreply.github.com>
@DanielHe4rt DanielHe4rt merged commit d783524 into v2/main Feb 11, 2023
$this->level >= 50 => 2.0
};
$memberStatusMultiplier = $isSupporter ? 0.25 : 0.4;
$experienceObtained = ($states->getExperienceMultiplier() / ($this->level * $memberStatusMultiplier) * 20);
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix parentheses order or remove them

Suggested change
$experienceObtained = ($states->getExperienceMultiplier() / ($this->level * $memberStatusMultiplier) * 20);
$experienceObtained = ($states->getExperienceMultiplier() / ($this->level * $memberStatusMultiplier)) * 20;

Comment on lines +14 to +16
self::Disabled => 0,
self::Muted => 3,
self::Unmuted => 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed with @EduardoRFS before, using larger multipliers make easier to make subtle changes. For example, imagine you want to decrease the unmuted multiplier by 10%. We would need to work with floating numbers to make it possible.

In resume, it would be much easier to work with larger numbers, since no casting is needed.

Suggested change
self::Disabled => 0,
self::Muted => 3,
self::Unmuted => 5,
self::Disabled => 0,
self::Muted => 30,
self::Unmuted => 50,

Comment on lines +53 to +56
public function getLevelAttribute(): int
{
return (new LevelEntity($this->experience))->getLevel();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather use the new accessor format. Define the level attribute on the DocBlock too, please.

Suggested change
public function getLevelAttribute(): int
{
return (new LevelEntity($this->experience))->getLevel();
}
public function level(): Attribute
{
return Attribute::make(
get: fn () => (new LevelEntity($this->experience))->getLevel()
);
}

{
}

public function persist(array $payload): void
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that we don't have time for it, but it would be sick to have a ViewModel to represent this payload. Working with arrays are kinda cringe, since we don't have "proof" that the accessed value exists.

{
}

public function persist(array $payload): void
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that we don't have time for it, but it would be sick to have something like a VoiceMessageInput object to represent this payload. Working with arrays are kinda cringe, since we don't have "proof" that the accessed value exists.

private ?string $country,
private ?string $state,
private ?string $city,
private ?int $zipCode,
) {
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think is nice to move the creation logic to the model layer $addressModel->toEntity(). But it's just another suggestion, it's fine as it is right now.

Comment on lines +19 to +23
$response = $action->handle(
ProviderEnum::from($provider),
$request->input('provider_id'),
$request->input('username')
);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the database is down? Shouldn't we handle this situations and define responses for them?

) {
}

public function handle()
Copy link
Contributor

Choose a reason for hiding this comment

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

What handle means?

) {
}

public function handle(string $value, array $payload): void
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename handle method.

Comment on lines +10 to +15
private ?string $name,
private ?string $nickname,
private ?string $linkedinUrl,
private ?string $githubUrl,
private ?string $birthdate,
private ?string $about,
Copy link
Contributor

Choose a reason for hiding this comment

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

If your goal was to make this properties nullables, you should also define the fallback value as = null

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.

None yet

3 participants