
My most common code review suggestions - grey_shirts
https://hackernoon.com/what-i-learned-from-doing-1000-code-reviews-fe28d4d11c71
======
draluy
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...](https://stackoverflow.com/questions/26327957/should-
java-8-getters-return-optional-type/26328555#26328555)), 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.

~~~
virgilp
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.

~~~
tobyhinloopen
They cannot truly fix the problem without dropping backwards compatibility.

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

~~~
dep_b
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.

~~~
yoz-y
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.

------
fjfaase
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.

~~~
qznc
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.

------
unwind
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. :(

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

------
alanfranzoni
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.

~~~
bonesss
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.

~~~
alanfranzoni
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.

------
dep_b
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

~~~
bonesss
Just for clarity: what you've described are (Units of
Measure)[[https://docs.microsoft.com/en-us/dotnet/fsharp/language-
refe...](https://docs.microsoft.com/en-us/dotnet/fsharp/language-
reference/units-of-measure)], 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

~~~
dep_b
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!

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

~~~
icebraining
Not being completely specific is not "clickbait".

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

------
Tade0
> 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.

~~~
ben-schaaf
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.

~~~
dozzie
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.

------
pasta
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](https://www.youtube.com/watch?v=igFRvriDMqk)
(GOTO 2015 • Secure Coding Patterns • Andreas Hallberg)

------
astura
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.

------
nautilus12
In summary: Use Scala instead of Java

------
meribold
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.

~~~
CannisterFlux
I use this guy's "kill sticky" bookmarklet for cases like this
[https://alisdair.mcdiarmid.org/kill-sticky-
headers/](https://alisdair.mcdiarmid.org/kill-sticky-headers/)

------
Annatar
_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.

~~~
TeMPOraL
> _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.

