Code review
Created by: Gondolav
Code review
This is a code review for the code of the whole project on master branch at commit https://github.com/H-PixelDroid/PixelDroid/tree/92c534ca1bb4781d0b049c353d0160e7310df558. Of course, we don't expect you to change all of your code (some parts of the review can be subjective or arguable anyway), what’s important is that you understand why better alternatives may exist.
Please ask us if you have any question about the code review, whether it is by writing a comment below or by sending us an email or meeting us.
Important: if you try to fix these, don't break your app. We prefer that the app works even if you don't manage to fix anything. It may be too hard to rework many of the issues below, as it would require to rewrite the whole architecture.
General remarks
Most of the general remarks are listed here; only very file-specific remarks are listed under each file section.
- Prefer
val
overvar
whenever possible. - Prefer lifting the
return
out ofif
s. - Always choose the access modifiers as restrictive as possible.
- Try to take advantage of Android Studio’s suggestions, they are often useful.
- You can (and should) run the Android linter to spot errors and warnings (Analyze -> Inspect Code...).
- You should have done a singleton for the
PixelfedAPI
object instead of recreating it in each class that uses it. It is preferable in terms of memory usage and code simplicity. - You put sometimes String literals directly into the code. You should always use Android resources for Strings that will appear in the UI. It is useful especially for translation but also just for “having everything at the same place”. See Android Linter for more. That is for XML and Kotlin files.
- You can easily format your code by using the Android Studio formatter in Code > Reformat code.
- If you want to use view binding, then you should try to stick to it in every activity.
- You should avoid using magic numbers in your code (just like string literals hard coded). Define constants instead.
Classes
Package api
PixelfedAPI.kt
- Several single-line comments could be included as KDoc comments for each function.
Package db
AppDatabase.kt
-
instance
can be a val. - You have a variable
TEST_MODE
to indicate whether the code is executed inside a test or in production. This is really bad. You must not have code in your production code base that is exclusively related to testing. You should use dependency injection to inject aninMemory
Database instance instantiated in test files.
Converters.kt
- The conversion with
toLong()
is useless.
PostDao.kt
PostEntity.kt
- Never use in column names (see SQL naming conventions for more).
- Columns names and table name should be defined as constants elsewhere, for example inside PostDao.kt.
Package fragments
Subpackage feeds
FeedFragment.kt
- Weirdly formatted at times.
-
content
andfactory
should beprotected
as well.
FollowsFragment.kt
- Inside
makeInitialCall
andmakeAfterCall
you can lift out the return statement. -
profilePicRequest
can be private.
HomeFragment.kt
-
picRequest
can be private. - Inside
HomeRecyclerViewAdapter
’sonBindViewHolder
, you can useapply {}
to call all those methods on post.
NotificationsFragment.kt
-
profilePicRequest
can be private.
NewPostFragment.kt
PostFragment.kt
- You can use
apply {}
for status.
ProfileFragment.kt
- Avoid magic numbers, extract out constants instead (spanCount when building GridLayoutManager in
onCreateView()
and response code 200). For the response code this is not the first time you use it, maybe it is worth extracting it as a constant (or enum) somewhere else and make it available where needed.
ProfilePostsFragment.kt
-
onCreate()
is useless, as it only calls the superclass’s method. - The keys used in key-value pairs retrieval (here we speak about key
”domain”
inaccessToken
definition) should be constants defined one for all.
ProfilePostsRecyclerViewAdapter.kt
Package objects
Account.kt
- Same comment as before about magic values (http response code).
- Always use Android resources for Strings displayed on the UI for translating purpose (buttons, toasts, …).
Application.kt
Attachment.kt
Card.kt
Context.kt
Emoji.kt
FeedContent.kt
-
equals()
is useless, as it only calls the superclass’s method.
Field.kt
- Empty.
History.kt
Instance.kt
Mention.kt
Notification.kt
Poll.kt
- Empty.
Relationship.kt
Source.kt
- Empty.
Status.kt
- You can use
apply {}
in different places.- e.g. in
setupPost
you don’t even need to define aval
, you can directly callapply{}
onfindViewById<>()
.
- e.g. in
Tag.kt
Token.kt
Package utils
Subpackage customSpans
ClickableSpanNoUnderline.kt
DatabaseUtils.kt
- You don’t need to create utility classes with “static” methods in companion objects. Instead, in Kotlin you can just define extension functions inside files (without any class wrapping them). If you’re curious you can ask us, or just google them!
HtmlUtils.kt
- You can define constants for Strings and chars like “www.”, ‘#’, ‘@’, etc..
ImageConverter.kt
PostUtils.kt
- Same comment as before about magic numbers and String constants that are displayed: use constants for numbers and Android resources for displayed Strings.
Root package
FollowsActivity.kt
-
followsFragment
can be private. - you can use syntax this kind of syntax for null check instead of if:
a?.let {
println("not null")
println("Wop-bop-a-loom-a-boom-bam-boom")
} ?: run {
println("null")
println("When things go null, don't go with them")
}
it is not mandatory but it exploits more the beautiful and lighter syntax of Kotlin.
ImageFragment.kt
- It’s better to define those constants inside the companion object, rather than defining them at the top level, outside the class.
LoginActivity.kt
- In
normalizeDomain()
you can remove the var and simply chain function calls in the return statement. - You should use constants for keys when adding values in
preferences.edit()
.
MainActivity.kt
PostActivity.kt
-
postFragment
can be private.
PostCreationActivity.kt
- You can use
apply {}
insaveImage()
. - Use Android resources for String that are displayed.
ProfileActivity.kt
-
profileFragment
can be private.
SettingsActivity.kt
Tests
- Testing code should be as readable as production code.
- All tests should meaningfully assert conditions or check useful properties.
- You should follow the same package structure you have for the real files.
- Some fields can be private.
Conclusion
Good work overall! Some small issues are present but the project is overall really well done. You should take more advantage of the Kotlin syntax and the cool stuff it provides, but it is not really a problem. It is more an opportunity to learn and be more productive.