-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: dev
Are you sure you want to change the base?
[#147]Convert to Kotlin- data/remote/model/NoteRemote.java #191
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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