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.

Sign up for GitHub

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

Jump to bottom

Added .properties file #31

Merged
merged 19 commits into from
Jun 16, 2024
Merged

Added .properties file #31

merged 19 commits into from
Jun 16, 2024

Conversation

welleyth
Copy link
Collaborator

No description provided.

@welleyth welleyth linked an issue Jun 10, 2024 that may be closed by this pull request
Use properties instead of some constants #15
Closed
MAKMED1337
MAKMED1337 requested changes Jun 13, 2024
View reviewed changes
core/src/com/po/fuck/model/.properties Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/Core.java Outdated Show resolved Hide resolved
gradle/wrapper/gradle-wrapper.properties Show resolved Hide resolved
@welleyth welleyth requested a review from MAKMED1337 June 16, 2024 00:18
MAKMED1337
MAKMED1337 requested changes Jun 16, 2024
View reviewed changes
Copy link
Owner

@MAKMED1337 MAKMED1337 left a comment

Choose a reason for hiding this comment

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

Why are you commenting properties like # borders.properties ?

core/src/com/po/fuck/model/Constants.java Outdated Show resolved Hide resolved
@welleyth welleyth requested a review from MAKMED1337 June 16, 2024 11:22
MAKMED1337
MAKMED1337 requested changes Jun 16, 2024
View reviewed changes
Copy link
Owner

@MAKMED1337 MAKMED1337 left a comment

Choose a reason for hiding this comment

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

Few visibility modifiers were missed.
Also I am not getting idea with static getProperties.
Why are you adding comment to constants like # enemy.properties ?

core/src/com/po/fuck/model/constants/BalanceConstants.java Show resolved Hide resolved
core/src/com/po/fuck/model/constants/BalanceConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/BaseConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/BalanceConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/ConstantsLoader.java Show resolved Hide resolved
core/src/com/po/fuck/model/constants/ConstantsLoader.java
Comment on lines +20 to +21
if(value == null)
throw new RuntimeException(name + " is null");
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure we don't want to have some constants to be null ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why would we?

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe null means to disable some feature. For example: CHAT_COOLDOWN = null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Null is not intended

core/src/com/po/fuck/model/constants/ConstantsSaver.java Outdated Show resolved Hide resolved
@welleyth welleyth requested a review from MAKMED1337 June 16, 2024 13:35
MAKMED1337
MAKMED1337 requested changes Jun 16, 2024
View reviewed changes
Copy link
Owner

@MAKMED1337 MAKMED1337 left a comment

Choose a reason for hiding this comment

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

image

core/src/com/po/fuck/model/constants/BalanceConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/BalanceConstants.java Show resolved Hide resolved
@welleyth
Copy link
Collaborator Author

welleyth commented Jun 16, 2024 •
edited
Loading

What the

@welleyth welleyth requested a review from MAKMED1337 June 16, 2024 17:47
MAKMED1337
MAKMED1337 requested changes Jun 16, 2024
View reviewed changes
Copy link
Owner

@MAKMED1337 MAKMED1337 left a comment

Choose a reason for hiding this comment

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

Overall OK, but prefixing path and properties does not sound very nice, and some variable prefixing/suffixing is unnecessary, because they are already inside of the class.

core/src/com/po/fuck/model/constants/PhysicsConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/LayeringConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/GameConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/CameraConstants.java Outdated Show resolved Hide resolved
core/src/com/po/fuck/model/constants/BordersConstants.java Outdated
import static com.po.fuck.model.constants.ConstantsLoader.loadInt;


public final class BordersConstants extends BaseConstants {
Copy link
Owner

Choose a reason for hiding this comment

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

Do they need their own constants ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. Can move it to Position ?

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds not very great, because then we need to do something with speed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

?

Copy link
Owner

Choose a reason for hiding this comment

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

If we have position constants, then the speed should be there also, because it also depends on a lengths/etc.

Copy link
Owner

Choose a reason for hiding this comment

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

Can we simply remove borders ?

@welleyth welleyth requested a review from MAKMED1337 June 16, 2024 19:57
MAKMED1337
MAKMED1337 reviewed Jun 16, 2024
View reviewed changes
Copy link
Owner

@MAKMED1337 MAKMED1337 left a comment

Choose a reason for hiding this comment

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

Overall OK, but some conversations left.

@welleyth welleyth requested a review from MAKMED1337 June 16, 2024 21:13
@welleyth welleyth merged commit 1da120b into main Jun 16, 2024
@welleyth welleyth deleted the 15-use-properties-instead-of-some-constants branch June 16, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyhorKoshman RyhorKoshman RyhorKoshman approved these changes

@MAKMED1337 MAKMED1337 MAKMED1337 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use properties instead of some constants
3 participants
@welleyth @MAKMED1337 @RyhorKoshman