Skip to content

Code review#1

Open
Happy0-0 wants to merge 2 commits intomikeal200:masterfrom
Happy0-0:CodeReview
Open

Code review#1
Happy0-0 wants to merge 2 commits intomikeal200:masterfrom
Happy0-0:CodeReview

Conversation

@Happy0-0
Copy link
Copy Markdown

@Happy0-0 Happy0-0 commented Mar 9, 2020

Description of the Project:
The project is called Party Player but I found no evidence of any function that was related to this.

What I learned from the code review:
*I learned that you need to add circleCI integration to a project.
*I learned that you need you can't have a functional program if you don't have any relevant code in it.
*I learned that to make your program function the way that you want it to, you must add the correct code to make it do that.

@@ -1,8 +1,5 @@
<component name="ProjectCodeStyleConfiguration">
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

need to add relevant phone permissions to project.

@Happy0-0
Copy link
Copy Markdown
Author

Happy0-0 commented Mar 9, 2020

Analysis of the program:
-the project has almost no code in it and has no code that can be relevant to the name of the project.
-the program was available on time.
-the program is commented for the little bit of code that they did have.
-the program does compile.
-I suggested these changes because they are standard for what needs to be done for the app to progress.

Technical concepts to add:
-needs to have circleci pipeline integration
-needs to have android permissions added
-needs to have firebase integration added.
-needs to have basic code that is relevant to the name.
-needs to have google.services added.

Comment thread .idea/gradle.xml
@@ -1,8 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

needs to add firebase support

class ExampleInstrumentedTest {
@Test
fun useAppContext() {
// Context of the app under test.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Needs to add more context and more tests to the app.

@discospiff
Copy link
Copy Markdown

There aren't code changes committed in this pull request, so there's nothing to merge... thus, I don't recommend merging.

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