My most common code review suggestions | Hacker News Reader

My most common code review suggestions

hackernoon.com

44 pointsgrey_shirts49 comments

I understand most of your points, but I disagree when it comes to the Optional part of your article. As stated by Brian Goetz here (https://stackoverflow.com/questions/26327957/should-java-8-g...), the intent was not to use optional all the time as you suggest, going as far as to say "Optional allows you to completely remove NPEs from your program. ". No they do not, an optional can be null anyway. As explained by Mr Goetz, they should only be used when designing an API, to explain to the consumer that a result may not be returned: "you probably should never use it for something that returns an array of results, or a list of results; instead return an empty array or list. You should almost never use it as a field of something or a method parameter."

I think we should keep in mind this is a type added for the JDK code foremost, and use it in similar use cases, and not see it as a general replacement for nullable values.

The big problem with java is that you don't have non-nullable versions for most types; in my mind, the biggest advantage of Option is the default implication that "everything that's not explicitly Optional is really non-optional". Alas, that's not true in Java - if you have a non-optional type, you can (almost) never be sure that the value is non-null. It feels like just adding Optional, they only did half of the job.

They cannot truly fix the problem without dropping backwards compatibility.

Could the add some non-nullable annotation like apple did when updating objective-c to play nicely with Swift's optionals?

Non-nullable is a promise not a guarantee. It does absolutely nothing in terms of useful warnings or compiler errors within Objective-C, it only explains how Swift should consume the Objective-C API.

It's still entirely possible to get a nil from Objective-C while the API is declared as nonnullable, this even happened to me with Apple's own API's if abused to a certain limit.

Yes but if Java 10 (for example) introduced this annotation then the JVM10 could transform a null returned from a non-nullable annotated function to an exception. Compile time static checks could also be added to guarantee that such a method does not return null. But you could still use such a function from a previous java version albeit you would sacrifice the checks.

It is a way to introduce the benefits (as long as the library is developed properly) of non-nullable types without sacrificing retro compatibility.

Edit: I remember a bug in earlier version of Swift Cocoa binding where getting calendar events without titles would hang without possibility of recourse because of this problems.

Something like what they are trying for C# may work for Java too: https://blogs.msdn.microsoft.com/dotnet/2017/11/15/nullable-...

I'm not sure if it is going mainstream right away, but C# is trying an interesting solution to that same problem: reversing their default relationship with 'nulls' and adding in optional syntax to flag variables as 'nullable'.

In terms of backwards compatibility the language teams position, IIRC, was that in the cases where you are intentionally using null it's quite easy to find and flag flag the nullable variable, but in all other cases the unexpected nullability can responsibly be removed from apps, making then-unnecessary null checks superfluous.

It's a bold move, but if it works at scale Java might be able to pull the tablecloth out from under the plates too...

That's only partially true (and it is harder to do now that they introduced Optional).

Think about it like this: in Java - all class references were Option<Class> by default - and what was truly lacking was the possibility of having a non-optional reference[1]. They could've treated transparently all old-code[2] references as Option<ClassName>, and just given you the possibility of having non-optional reference in the new code (that meant that you can't pass objects with non-optional references to the legacy libraries, but it's a pain that could've gone away pretty quickly as libraries got recompiled to the new java versions)

[1] Oracle's interpretation was the other way around: "Java lacks the Option type, let's add it". I argue they got it wrong - Java always had the Option type, it's just that it was implicit; and since Java was lacking the non-optional types, you couldn't do anything useful with the Option, other than check for "None" (i.e. null).

[2] By "old code" I mean "old compiled code". You keep the binary compatibility - but indeed, you would need to drop the source-level compatibility, since compiling old code with the new compiler would not have worked. But, again, that would be fixable with a "legacy syntax" compiler flag, that would produce old-style binaries from old-style source code.

Absolutely. So, what I started to do in my java code: Before dereferncing anything for the first time, i add

    assert foo != null : "foo must not be null - remember to wrap optional data in Optional<T>"

Just make sure to run your program (or at least the tests) with the JVM flag "-ea"

What is the point? How is an assert better than a NullPointerException?

If asserts are disabled, then the null pointer checks are still inserted by the JVM.

The benefit of using an assert is that it makes the error condition known immediately to the caller and the reason. Otherwise, whose to say when the NPE will occur. It could be raised just a couple lines down, or it could be a couple methods or a classes away which increases the complexity of diagnosing the actual cause of the error.

A more idiomatic approach would be Objects.notNull(foo)

In this case the most pragmatic thing to do is to ensure nothing in your application can ever be null - use asserts during development, empty lists, null objects, etc. Also never catch NPE's, if there is one after all those precautions, it's a developer error / bug and should be punished as such.

The problem with optionals - as Sonar will complain about - is that people will call `.get()` on an Optional without first checking `.isPresent()` for example.

Lots of things in the language that allow you to shoot yourself in the foot that other languages have solved.

Semantically, is that different from using a reference variable without first checking it !=null?

Also, isn't there a static code analysis tool that checks at compile-time that reference variables annotated as @non_nullable can never be null?

There is exactly one place where you might want to catch an NPE: top level global exception handler for logging and debugging.

Returning an empty list or empty array is different from returning no list at all.

Imagine I want to query a database of tagged articles. Let's say I want to get articles with an exact set of tags.

Passing this method [A, B] will give me articles with tags A and B, passing this method [] will give me articles with no tags. Not specifying the query parameter at all will result in no parameter. I could use a boolean value to keep track of that or just use an optional.

The point is that returning an Optional gives you nothing above returning a null - in fact it increases complexity and makes calling code more error-prone for no gain.

If your contract says you return a Foo, you could be returning a null. If you change that to Optional<Foo> you could still be returning a null.

It's not what Optionals are for. They are just widely misunderstood and abused.

Thank you for calling this out. I have to fight this fight way too often.

Optional is really for functional code, not for "eliminating NPEs," because it doesn't do that - in fact it means strictly speaking you have to do additional checking.

The real purpose of optional is for allowing me to do things like `foo.first().orElseGet(other)`. `first` has to return an Optional rather than a null or it will blow up.

If you are doing `Optional.isPresent()` as a straight-up replacement for null checks then you are doing it wrong. If you want to avoid returning nulls and doing null checks, use polymorphic null objects.

When seeing the link, I had hoped to read some general wisdom about the benefits of reviewing code or some advice about the best way to perform a code review, not some language specific remarks about Java usage.

I made myself a checklist of reviewing pull requests to dlangs standard library. The only general item is "understood comments in code". The rest is specific.

The section titles are kind of generic as they convey the aspects to consider: Code Gotchas, Documentation, Code Style, Unittests.

This is off-topic/meta, but I'm still surprised every time I've read something on Medium, and my (desktop Firefox) back button doesn't take me back. Whyyyy do sites still do this?! Yes, that's rhetorical. :(

which back button? the browser back button? are you on mobile? anyway it opens a new tab, so no back

I just tried on firefox 57, it does take me back, strange

Point 2 is something that gets overlooked easily nowadays. I'd use even more specific types (or 'tiny types' as they're called sometimes), like MemberCompanyId, which could hold a Company instance and an Integer instance. Such members could be validated, i.e. checked for not null and for valid values (the id number could be forced positive, as an example). This will only allow to construct valid MemberCompanyId instances, and prevent any programming error caused by accidental deviations in input or state that create invalid objects.

Point 3: interesting and useful, I had never thought about it. I use varargs syntax quite often by the way, letting the caller pick what he wants to do with my code.

I also use "tiny types" a lot. In languages without any typing support (EG Javascript), I use ESDoc to write comments about expected types everywhere in my codebase, and I have a lot of `@typedef`'s, like `@typedef {string} CompanyID`.

I tried things like TypeScript and Flow, but I didn't get it working nicely with JSDoc or ESDoc. I want type-checking & generated docs, and I don't wanna define both twice.

I find it kinda odd that a fundamental principle of OOP is information hiding, yet most mainstream languages make it a matter of pure discipline if you want to pass around a "CustomerId" instead of an "Int32" or alias any value type. In your own code it's not too bad, but there's constant conversions at api and library boundaries...

In general I lack the Smalltalk ideal of being able to subclass any type, like 'Int', call it something new and attach behaviour to it, while preserving API & DB compatibility.

Type aliasing in F#, ie `type Customer = int`, or discriminated unions with the same info, like `type CustomerId = CustomerId of int`, are wonderfully useful in that regard.

In Python you can subclass builtin types and override __new__. Not every method can be subclassed with success, though, because of optimizations.

I agree it's the way to go, though. My typical tiny type in Python just has preconditions on string content.

Stronger typing is more secure code, totally agreed. F#'s custom primitive types (I'm corrected, it's Units of Measure) are a feature that should be copied by every language. You're not passing around floats anymore with a descriptive name ('distance') but they're floats defined as kilometers or miles. Then you can define rules about how they should interact, like:

    Mile distance1 = 12
    Kilometer distance2 = Kilometer(distance1) // Does a conversion while constructing

    void someFuncThatWantsKilometers(Kilometer distance) { }

    someFuncThatWantsKilometers(distance1) // Does not compile
    someFuncThatWantsKilometers(distance2) // Compiles

Even better you can do this:

    Hour time = 1
    Kilometer distance = 12
    Kilometer/Hour speed = time/distance // Doesn't compile
    Kilometer/Hour speed = distance/time // Compiles! Even gives you the 'new' type for free!

You could use it for everything where primitives have a deeper meaning than their pure numeric value. Database ID's, money or other financial or scientific values. Impossible to swap X and Y coordinates unless you implicitly do so.

You can eliminate so many confusing bugs just by having better typed primitives.

--- disclaimer: ---

This is not exactly how it works in F# but the languages are too different to really compare

Just for clarity: what you've described are (Units of Measure)[https://docs.microsoft.com/en-us/dotnet/fsharp/language-refe...], which let you define types, and their relationships, so you can define meters, and kilometers as meters * 1000 and then rest safe that your spaceshuttle will be using the right units.

Unit of Measure in practice: [<Measure>] type cm [<Measure>] type ml = cm^3 // a millilitre is 1 cm cubed

To the point in the article about using types to convey meaning, as I commented elsewhere, you want F#s awesome type aliasing and single-case discriminated unions :)

Type aliasing:

   type CustomerId = int // We can send now use IDs interchangeably with int, use CustomerId in method signatures, and expand the type with methods

Single Case Discriminated Unions:

   type CustomerId = CustomerId of int
   let id = CustomerId (3)           // Have to use a specific constructor
   let myFunction (CustomerId(id)) = // DUs can be unpacked in parameters
       printfn "%i" id               // this function only accepts CustomerId's

That's what it was called, sorry I was citing from the top of my head. I don't find the name Units of Measure a bit confusing as it seems to imply that it's only use is scientific units while there are plenty of use cases outside of that realm.

A SandwichCount should not be confused with a BurgerCount!

That's one of two things I always dreamed to have attached to numeric values and used/propagated automatically by a programming language. The second thing is error bars.

I will start by saying I agree with you, and also that many functional languages bring types to the next level, f# included.

But I will also say this is possible in other languages to some degree with some level of boiler plate.

For example, in java I can do similar. Instead of using float, wrap that in a class called kilometer and use that instead. Then define the api around it as needed (equals, compare, value, etc). Even this small thing I think is really important, and is helpful instead of using primitives or strings. Make email instead of string, dollar instead of just big decimal, etc.

If it's hard to use people tend to avoid using it. Also the wrapped int would have to be private to avoid people to use it anyway.

"What I learned from doing 1000 _java_ code reviews" - there, clickbait fixed, irrelevant tab closed.

Not being completely specific is not "clickbait".

"This guy did 1000 code reviews and you'll NEVER guess what he found at #123!"

> and is basically the entire reason for choosing strongly typed language like Java in the first place.

Technical nitpicking: It's best to avoid using the terms "strong" and "weak" when referring to type systems, since they don't have clear, agreed upon definitions.

Some would say that Java is not a strongly typed language since its type system isn't sound.

Just to add to this, the better terms to use are "static" and "dynamic".

Going with the example of a sound type system, a language could have a sound type system that does the actual checking at runtime.

No, not really. Even though there's no good definition what "strong" and "weak" means, it was always meant as a totally different axis than static/dynamic typing. C is weakly typed, OCaml is strongly typed. Both are typed statically.

A type soundness theorem is always of the form “a well-typed program never does such and such”. It relates the static semantics (type checking) to the dynamic semantics (operational semantics) of a programming language.

It doesn't make any sense to talk about soundness for dynamic languages, because there's no static semantics to relate to the dynamic semantics.

Andreas Hallberg has a great talk about this[1]: crash early, use explicit types (value objects) and so on.

[1] https://www.youtube.com/watch?v=igFRvriDMqk (GOTO 2015 • Secure Coding Patterns • Andreas Hallberg)

I hope this author is only doing code reviews for very junior engineers, number one and number two are only mistakes I would expect from someone very inexperienced.

As pointed out elsewhere here, the author has Optional wrong.

In summary: Use Scala instead of Java

The overlay navigation bar at the top is really annoying and now there's another bar at the bottom nagging me to sign up for something that can't be removed.

I use this guy's "kill sticky" bookmarklet for cases like this https://alisdair.mcdiarmid.org/kill-sticky-headers/

If something is exceptional you should throw an exception.

This is so stereotypical of the object-oriented mindset and Java programmers are the worst offenders. If I had a programmer under me who came up with this, I would fire them on the spot and enjoy every nanosecond while doing it.

The “punt and throw an exception” mentality goes directly against the art of UNIX programming rule of repair: repair what you can, but if you must fail, do it as soon and as noisily as possible. What this means in practice is that in 99% of the cases, the programmer could have written the code to either deal with the problem or had enough information from disparate sources to reconstruct the missing data but they chose to punt and throw an exception instead. To me as a programmer, that is infuriating!

Which leads me to point 2.: the exception paradigm requires the intimate knowledge of the software to interpret, as it normally vomits a call stack trace. But in production, to the user of the application or the poor system administrator, or even to the programmer who isn’t intimately familiar with the software, that information is nothing but retching vomit, often with no clues as to what went wrong where! And did you notice how the punter is never around to decipher the gobledygook when this happens? Also infuriating.

Stop throwing exceptions, get off of your lazy asses and when you can reconstruct the missing information, or detect and correct damaged information, or diagnose the problem, do so and give back meaningful diagnostic telemetry on stderr, but don’t just punt and screw the user or the sysadmin over.

If you’re programming to put the bread on the table, it’s not someone else job to make it work; it’s yours. That’s why you get paid, to make the computer compute. No excuses.

> but if you must fail, do it as soon and as noisily as possible.

So basically, spam on stderr and abort(). This is not exactly better than exceptions.

> the exception paradigm requires the intimate knowledge of the software to interpret, as it normally vomits a call stack trace. But in production, to the user of the application or the poor system administrator, or even to the programmer who isn’t intimately familiar with the software, that information is nothing but retching vomit, often with no clues as to what went wrong where!

Again, it's printing stuff to stderr, or having the exception system print stuff plus stack trace to stderr. The latter is strictly better than the former. It gives you more clue than printing what would otherwise be an exception message to stderr and aborting on the spot.

And using exceptions do not preclude you from being able to "repair what you can". In fact, it can help it.

I don't like exception abuse either, but it's still better than the C way.

You are ranting against something the author absolutely did not say.

Reply on news.ycombinator.com