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

Laurislopata/upload image #184

Merged
merged 12 commits into from
Apr 19, 2022
Merged

Laurislopata/upload image #184

merged 12 commits into from
Apr 19, 2022

Conversation

laurislopata
Copy link
Collaborator

@laurislopata laurislopata commented Apr 17, 2022

In this PR, I added a simple image selection and storage to firebase. Due to a lack of time this week I was unable to work more on it (connecting it with the user profile). I will finish that over the break as to why the coverage is low, these functions mainly use firebase and built-in activities like opening the default photos viewing app.

}


private fun updateUI() {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@laurislopata laurislopata marked this pull request as ready for review April 17, 2022 17:37
@jiabaow jiabaow self-requested a review April 18, 2022 09:04
Copy link
Collaborator

@jiabaow jiabaow left a comment

Choose a reason for hiding this comment

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

I left 2 minor remarks relating to documentation and code-formatting (for higher coverage). But since there will be a second PR adding more tests, the current state LGTM.


// taskSnapshot.metadata contains file metadata such as size, content-type, etc.
uploadTask.addOnSuccessListener { taskSnapshot ->
callback()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you will add the task later but you can put everything in one line here since the callback() is short, it will not affect the readability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Also, since testing these calls is hard, maybe consider concatenating some operations or lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also agreed, it's an easy way to make the coverage of external calls better.


class Util {
companion object {
fun createNewId(): String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider adding documentation for this (explaining the form of the id, etc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed.



private fun updateUI() {
findViewById<TextView>(R.id.uploadStatus).text = "Upload OK"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use strings.xml resources here instead of hardcoding. Let's keep it clean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed.

Copy link
Collaborator

@Tsathogguaa Tsathogguaa left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. If you can quickly remove the hardcoded string for convenience sake, and check if you can shorten the untested code lines, it'd be amazing.

Copy link
Collaborator

@zwierski zwierski left a comment

Choose a reason for hiding this comment

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

LGTM. The coverage is not an issue since you will do another PR with more tests, but in the meantime you can squish into fewer lines some functions that you cannot test.

@codeclimate
Copy link

codeclimate bot commented Apr 19, 2022

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

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

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

View more on Code Climate.

@laurislopata laurislopata merged commit c8704f6 into main Apr 19, 2022
@laurislopata laurislopata deleted the laurislopata/upload-image branch April 19, 2022 21:43
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.

4 participants