Another readability issue is not using trailing commas in method signatures and their invocations if they span multiple lines. The problem with this is that if we are to add a new parameter in the future the reviewer will see 2 lines of diff instead of 1 making the review harder. Furthermore the git blame will say that we are the one who added the previous parameter. As kotlin supports trailing commas I highly recommend thinking about using them.
I would create sealed interface to express authentication result. - Success - Error with message By the way, I enjoyed your CI/CD course👍 It was so great!
I always use named parameter except for short functions that are self describing. Example: Modifier.height(56.dp) instead of Modifier.height(height = 56.dp)
If you already took the time to extract if conditions to variables you might as well create third one "val shouldHighlightMessage = isLocalUserAdmin || areUserFriends", and call your sendMessage() function once, instead of two times. This will speed up reading process even more as developer would not need to go trough 2 "sendMessage" logics
I almost *never* use local variables like this; i'd much rather extract them into functions. You still end up with a nice 'named/semantic ' chunk of code but it's: 1. more readable/abstract; don't bother me with the implementation of deriving friends or admin, i only care about the result 2. it clearly limits scope of that result, reducing cognitive load 3. its reusable/extensible/composable/etc
Talking about named parameters: Intellij supports adding "parameter names" to java functions. It will then add comments that give you the name of the java parameter and while this does not give you any of the kotlin features, it's still very helpful
Love the video, and totally agree with your notes. Thanks! One small difference for me: I would pull the logic for local user admin and is friend to functions. I tend towards boundary testing, and that's much easier when those are broken into functions. There are other reasons as well, but I would imagine you would have already made it a function in those cases Again, thanks!
In a more general suggestion (despite being Kotlin or not), I would add; try to avoid very large files, not doing so makes the code reading hard/confusing/boring 😅, probably keeping the number of lines between 1k and 1.5k is still a good option (do so ONLY when needed, I mean, if your file is shorter than that, it's totally fine and great). Also large files might kill your Android Studio 🥲.
On the other extreme, don't go full enterprise Java and spread logic across 75 different files with 10 layers of inheritance so you have to go on a treasure hunt to figure out what every line of code actually does. There's usually a happy medium.
A good analogy for the null argument are use-cases like a calculator or a some monetary use-case, numbers should start with null instead of 0 or 0.0, but for a counter/counting use-case 0 should be a default value
Variable names are important but I don't think these examples are more readable with the added variables. I still will need to parse logic to see what logic is actually occurring.
I'd also avoid passing boolean parameters to functions as much as possible. Most of the times, it makes a lot more sense to create an enum and pass that instead.
@@PhilippLackner I think it's better if we had to extend the functionality later. For example, to add another user type BOT, other than FRIEND or ADMIN
Kotlin supports named parameters, so boolean parameters aren't that bad. For example, "Hello, World!".contains("hello", ignoreCase = true) is quite readable.
@@thebigboi5357 Enums make sense if you have a boolean where the negated value is unclear. For example isLeft. It's not clear what it means if isLeft is false, could be isRight, isTop, isBottom. But if the negated value is clear like for isLoading, isDropDownShowing etc. please use booleans
Maybe you should say something about whether performance is compromised by adding unnecesary variable definitions or other instructions that do nothing? is compiler getting rid of those automatically etc?
@@PhilippLackner Unless this function is called a billion times? ;) I don't know much about kotlin jvm etc, but I know a thing or two about "lowerish" level programming for very low power microcontrollers when you have to think 10 times if you really need this float or it can be done with just an int, I'm aware that CPU in average android device is at least million times faster than low power microcontroller and such thing in almost all cases will not be even detectable, but I think it should be said why that is, because otherwise we will end up with new generation of programmers that don't actually know what they are doing and that starts to be a plague in my biased opinion. I don't expect that high level android programmer will be fluent in ARM assembly, but it is important to know what you don't know.
@@ekstrapolatoraproksymujacy412 readability is most important, at the end you check performance of whole application (or your part of it) and only if you find a bottleneck you can sacrafice a bit of readability for faster code. Happens very rarely
"Don't be afraid to introduce more variables to make code more readable." I wish I could tattoo that on the heads of some of my old co-workers. I completely disagree with removing the comment. Keep it. If there's a bug, it's nice to know what the programmer was TRYING to do. This way we can tell if the algorithm is faulty or the implementation of it. Comments explain what you're trying to do; the code shows how it is accomplished. I've wasted hundreds of hours of my life debugging people's code; knowing the difference between the programmers' intent vs how it was implemented is invaluable.
If highlighted messages are a first class feature, would it not make more sense to have two send methods in the interface - send normal message and send highlighted message. Then you can lose the Boolean altogether
Good video! I just want to add, when using intellij IDE you can use the .val - shortcut to create a new val of an expression. Simply put ".val" behind the expression et voila.
The „areUsersFriends“ is not a good refactoring, you should rather move the list of „localUsers“ from the first part of the expression to a variable and keep the rest wirhin the condition. Not a fan of moving conditions to variables just to name them either, it bloats the code; just like the named parameters do. „checkIfAuthenticated“ is also bad naming, „checkAuthentication“ is suitable.
When you extracted the expression to a variable you should have done that using the built in receptions instead of doing it by hand so you cannot make a mistake.
Hey, I wanted to ask what monitor do you use for Android Studio? I have a 27" 4k only, and it's not enough to work with 4 columns in AS, so I'm wondering which one to buy to use as a central monitor or as second one to the current one.
Hey Phil, congrats on all the work you have done. Your job is fantastic, and you have been helping many developers with your content!! Maybe, another improvement, what about moving it.isLocalUser && it.isAdmin to a method inside the User class as one of its members, something like users.values.any {it.isLocalUserAdming()}? I noticed that people are no longer adding behaviours or using the state of an Object.
for the null, better add a boolean alongside or an enum when relevant. The rational behind that is that one variable should not encode two things. In the case of errorMessage being null, it encodes two things: whether there was an error and if so, the error message. With a boolean, you can do: if(auth.isError){ display(auth.errorMessage) } instead of: if(auth.errorMessage!=null){ display(auth.errorMessage) } Also, "==true" or "==false" should not be used. As for comments explaining "what", they also indicate that some code pertain to a function. I used that when doing QC ^^ And with Kotlin, you don't lose much with inline functions. So, it's worth to even put a one liner in a function.
With that argument nullables shouldn't be used at all, since they ALWAYS encode the existence of a value and the value if it exists. Can't agree with that 😅
