Skip to content
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.

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

[#147]Convert to Kotlin- data/remote/model/NoteRemote.java #191

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

OmneyaOsman
Copy link
Contributor

@OmneyaOsman OmneyaOsman commented Oct 22, 2019

1-Add new data kotlin class NoteRemote in data/remote/model package with fields provided in NoteRemote.java but in kotlin .
2-remove NoteRemote.java

@OmneyaOsman OmneyaOsman changed the title convert NoteRemote from java to kotlin [#147]Convert to Kotlin- data/remote/model/NoteRemote.java Oct 24, 2019
Copy link
Contributor

@victorrattis victorrattis left a comment

Choose a reason for hiding this comment

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

Please consider having one commit only for this pull request.

tip: Use git rebase -i HEAD~2 and do a push with -f to force the push.

Thank you for commits.

@SerializedName("description")
@Expose
public String description;
var description: String? = null,
@SerializedName("user")
@Expose(serialize = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why are you set the serialize value to false? I believe it is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it already exists in java class I didn't change it

@SerializedName("user")
@expose(serialize = false)
public String user;


@SerializedName("id")
@Expose
public String id;
var id: String? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please consider in each variable declaration, you could organize them on one line per one, in order to keep the same organization of the other data classes.
e.g:
change that:

@JvmField
@Expose
@SerializedName("id")
var id: String? = null

to that:
@JvmField @Expose @SerializedName("id") var id: String? = null


public NoteRemote(){}
}
var sessionId: String? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please consider using an empty string instead of null and repeat that to all the variables of this class.

e.g:
... var sessionId: String? = ""

var sessionId: String? = null
@SerializedName("id")
@Expose
@JvmField var id: String? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

You are using @JvmField to allow using "id" directly because there are code places (in Java) that are using the properties directly instead of using getter and setter methods.

Help me to decide what is a better way: Do you think better keep the @JvmField or remove it changing all the places that use these properties to use the getter and setter methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recognized that there is java code in the project so I have added @JvmField in order not to make any conflict, but in case the whole project will be I kotlin I recommend not to use it

1-Add new data kotlin class NoteRemote in data/remote/model package with fields provided in NoteRemote.java but in kotlin .
2-remove NoteRemote.java
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.

None yet

2 participants