Skip to content

My Code Review suggestions #2

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

Open
trentaml wants to merge 1 commit into
base: master
Choose a base branch
from
Open

My Code Review suggestions #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions ...e/party_player/ExampleInstrumentedTest.kt → ...m/example/partyPlayer/InstrumentedTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

Copy link
Copy Markdown

@discospiff discospiff Apr 16, 2020

Choose a reason for hiding this comment

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

While they are two words, package names are usually all lowercase. Consider making this all lowercase.


import androidx.test.platform.app.InstrumentationRegistry
import androidx.test.ext.junit.runners.AndroidJUnit4
Expand All @@ -10,11 +10,10 @@ import org.junit.Assert.*

/**
* Instrumented test, which will execute on an Android device.
*
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Cleaned up the comments by removing blank gaps.

* See [testing documentation](http://d.android.com/tools/testing).
*/
@RunWith(AndroidJUnit4::class)
class ExampleInstrumentedTest {
class InstrumentedTest {
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the name of the test files to be a little more visibly unique. (Both test files began with "Example")

@Test
fun useAppContext() {
// Context of the app under test.
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.example.party_player">
package="com.example.partyPlayer">
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html


<application
android:allowBackup="true"
Expand All @@ -9,7 +9,7 @@
android:roundIcon="@mipmap/ic_launcher_round"
android:supportsRtl="true"
android:theme="@style/AppTheme">
<activity android:name=".MainActivity">
<activity android:name="com.example.partyPlayer.MainActivity">
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

<intent-filter>
<action android:name="android.intent.action.MAIN" />

Expand Down
2 changes: 1 addition & 1 deletion .../com/example/party_player/MainActivity.kt → ...a/com/example/partyPlayer/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html


import androidx.appcompat.app.AppCompatActivity
import android.os.Bundle
Expand Down
5 changes: 2 additions & 3 deletions ...m/example/party_player/ExampleUnitTest.kt → .../java/com/example/partyPlayer/UnitTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html


import org.junit.Test

import org.junit.Assert.*

/**
* Example local unit test, which will execute on the development machine (host).
*
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Cleaned up the comments by removing blank gaps.

* See [testing documentation](http://d.android.com/tools/testing).
*/
class ExampleUnitTest {
class UnitTest {
Copy link
Copy Markdown
Collaborator Author

@trentaml trentaml Mar 13, 2020

Choose a reason for hiding this comment

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

Changed the name of the test files to be a little more visibly unique. (Both test files began with "Example")

Copy link
Copy Markdown

@discospiff discospiff Apr 16, 2020

Choose a reason for hiding this comment

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

While you're removing the "Example" prefix, rename the test to something that is more descriptive of what is being tested. "UnitTest" is implied here, based on the location of this class. What is being unit tested? Put that in the name of the class.

@Test
fun addition_isCorrect() {
assertEquals(4, 2 + 2)
Expand Down