Skip to content

My code review suggestions#4

Open
trentaml wants to merge 1 commit intomasterfrom
trentaml_CodeReview2
Open

My code review suggestions#4
trentaml wants to merge 1 commit intomasterfrom
trentaml_CodeReview2

Conversation

@trentaml
Copy link
Copy Markdown
Collaborator

@trentaml trentaml commented Apr 5, 2020

Project Description:
-This project is an app that creates a music playlist for parties based upon genre suggestions by users

What I learned reviewing this code:
-Proper naming conventions
-Proper comment syntax

There were files that were changed that I did not directly interact with. This might be due to a commit that was made to the master that was not included in the original Clone of the project

@@ -1,4 +1,4 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

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 package to not include underscores in accordance with naming conventions here:
https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, I'd take another option... all lowercase is typical for packages. Kotlin discourages multiple words.
"Names of packages are always lower case and do not use underscores (org.example.project). Using multi-word names is generally discouraged, but if you do need to use multiple words, you can either simply concatenate them together or use the camel case (org.example.myProject)."


} catch (e: JSONException) {
}
catch (e: JSONException) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Moved the catch statement off of the same line as the try statement ending curly brace

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The location of curlies has been a long debated topic in programming circles for decades. Either way, it's best to be consistent: if you have a separate line for curlies, put them all on a separate line. in other words, move the end curly on line 110 to a new line.

All in all, putting curlies on the same line as the catch appears to be convention.


class TrackService {

// fun fetchTracks(artistName)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

added space after the comment // in accordance with horizontal spacing conventions:
https://kotlinlang.org/docs/reference/coding-conventions.html#formatting

@discospiff
Copy link
Copy Markdown

In many cases, formatting can be a matter of personal preference. In lieu of merging this branch, I recommend using Android Studio's built-in standard formatter across all source files in your application: just press Control - Alt - L.

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.

2 participants