Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Finishing up offline game #310

Merged
merged 15 commits into from
May 19, 2022
Merged

Conversation

Tsathogguaa
Copy link
Collaborator

Finishing up offline game from last week. Testing on the emulator works fine, need to write actual tests, and go over some of the past tests that may need some changes as well.

@Tsathogguaa Tsathogguaa self-assigned this May 19, 2022

while(currentLine != null) {
while (currentLine != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good readability improvement

Copy link
Collaborator Author

@Tsathogguaa Tsathogguaa May 19, 2022

Choose a reason for hiding this comment

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

Thanks!

var trimmed = currentLine.trim()
val split = trimmed.split(" - ")

if (split[0].trim().lowercase() == buttonSplit[0].trim().lowercase()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add some comments explaining what split[0] and split[1] are?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, can you check if the explanations are clear?

Copy link
Collaborator

@laurislopata laurislopata left a comment

Choose a reason for hiding this comment

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

Added one comment you could address but overall really goo job :)

@MaximeZmt MaximeZmt self-requested a review May 19, 2022 20:49
Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

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

Nice work 👍 ! Just left tiny details in comments

@@ -125,6 +126,44 @@ class DeleteSongsActivity : AppCompatActivity() {
return false
}

private fun removeFromProperties(buttonText: String): Boolean {
Copy link
Owner

Choose a reason for hiding this comment

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

There is doc for all private function in this file. Maybe consider adding one line of javadoc for this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Changed it, committing soon!

Comment on lines 147 to 148
<string name="game_setup_internet_switch_on">Internet is on</string>
<string name="game_setup_internet_switch_off">Internet is off</string>
Copy link
Owner

Choose a reason for hiding this comment

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

As written in the wiki, consider using string name that respect string convention and put them at the top of the document in the corresponding part. (see here https://github.com/MaximeZmt/SDP_2022-Vibester/wiki)
Meaning

gameSetup_internetSwitchOn
gameSetup_internetSwitchOff

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Understood, will be in the next commit soon

Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

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

LGTM 👍 😃

@codeclimate
Copy link

codeclimate bot commented May 19, 2022

Code Climate has analyzed commit e3f041a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 93.8% (80% is the threshold).

This pull request will bring the total coverage in the repository to 85.5% (85.5% change).

View more on Code Climate.

@Tsathogguaa Tsathogguaa merged commit 86026c6 into main May 19, 2022
@Tsathogguaa Tsathogguaa deleted the tsathogguaa/offline_game_continued branch May 19, 2022 23:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants