Source
r/java•
springuni

What is wrong with this Java code? (Interview Question)

I've been asked this question on an interview and I'll also share what I answered.

I would like to know if there's anything I haven't thought of?

Java code:

if(person.getName() = “jan”) {return true;}

My answer was the following:

  1. If person is null, you'll get an NPE

  2. Using = is wrong, even == is wrong for strings, we don't look for the same reference but for the same value, equals would be adequate here like this: "jan".equals(person.getName())

  3. Something like person.getName() is rarely always lowercase, folks enter these sorts of things manually when they sign up. What if it's "Jan" instead or "JAN"? Should we return true then?

  4. What kind of user experience drives the design here? Why return true for a specific name?

Thanks for your help!

Posted Aug 28, 2020, 9:27 AM UTC

93105
myntt•

Just a tip for equality checks on all objects where you don't want to do new A().equals(b): use the Objects.equals() static method for null safety.

For Strings "compare".equals(s) is also fine. Depending on the context you might also want to trim or lower case first.

80
rzwitserloot•

Actually, no, don't do that as a blanket rule of thumb.

If null is just as much in the value domain as other values, that usually means you've messed up your design. null works best as representing "no value" or "unknown value". And just as the NaN double value is not considered equal to itself, and in SQL, null is not considered equal to itself, semantically, null ought not to be considered equal to anything, even another null value.

Crucially though, even "false" is wrong. When I ask you: is this candybar (I show you an empty open hand, acting as if there is a candybar in there) the same as the one in that closed drawer you can't open right now? Then the only right answer is "I do not know".

Both true and false are incorrect.

Throw the npe.

Years of culture, stack over flow posts, etc are all wrong. NPE is good.

41
liquidhot•

You make some good points but other times I just don't care. I'm really only interested if the value is jan. It could be any other value or no value. Throwing an NPE would just get in the way later, again, depending on intent.

20
rzwitserloot•

Sure. 'sometimes you care, sometimes you don't'. I started off that advice with: "Don't apply as blanket rule of thumb...", so we appear to be in agreement.

14
liquidhot•

Fair enough, I interpreted it as "use this as a blanket rule of thumb instead.", but I see now it's more of a "it's OK to use NPE". =)

5
springuniOP•

This is the most insightful comment, I've received. Thanks for it.

4
myntt•

Yeah that's a valid point, I definitely did not mean it as a rule of a thumb but rather as a tip if one needs it. I usually always secure subsystem borders with Objects.requireNonNull() and then ignore null checks or use optional instead if nothing is required as valid argument.

2
thephotoman•

I always hate it when I'm either catching or throwing NPEs with abandon. It feels so wrong. But sometimes, it's absolutely correct.

1
sunny_tomato_farm•

In my opinion, you should never be throwing or catching NPEs. A NPE is programmer error.

1
thephotoman•

There are plenty of times when null is a valid value. As a common and simple example: middle names. Not everybody has one, and when they don’t, you’ve got to handle that null value.

Your position is over-broad.

0
Shoox•

Years of culture, stack over flow posts, etc are all wrong. NPE is good.

I can't fully agree with that. The state of "no value" is needed but as far as language design goes, Kotlin's approach makes far more sense. Not-nullable should be the default state for every object and nullable objects should be declared explicitly and they should be the exception. The outer most layer needs to be able to handle null and any business logic behind that should not need to care about potential NPE.

Additionally, a NPE is a technicality, a bug where you expect a value but didn't find one. At the end of the line, someone didn't validate the input. You either expect and accept the state "no value" or the input is denied.

1
sunny_tomato_farm•

Fantastic post. If you look at my post above, I mention that NPE are programmer errors.

1
rzwitserloot•

NPE are often but not always programmer errors, yes. That makes them good! On a scoring system, for programmers errors:

  • At write time, the editor marks them with a red underline immediately: 100 points.
  • At compile time, the compiler will refuse to compile the code: 95 points.
  • At test or run time, the code WILL blow up with an exception trace pointing right at the problem: 85 points.
  • At test or run time, the code will silently do the wrong thing, or nothing: -1000 points.

The only thing 'better' about the kotlin model is something java cannot have (nor can kotlin code interopping with java code, for that matter), as it requires all methods to be explicit in marking down whether they can return null or not.

Kotlin got a free pass (moving from java to kotlin is the same impact as doing a python2 v python3 deal: You get to break backwards compatibility) and kotlin mostly squandered it; you could have beond more.

So, the kotlin note is irrelevant for the original question (if you don't think so I invite you to answer this interview question with: "What is wrong with this java code? It should be written in kotlin, that's what!" and see how far you get :P) - and as a driveby lets insert some feathers in kotlin's behind: Eh. I rather wish they'd have done more: Generics are 4 'modes' (covariant, contravariant, invariant, and 'legacy'). In kotlin, nullity should have been treated the same way, because all situations can legitimately occur. It didn't, and oversimplified matters.

1
A_random_zy•

we could also do it this way

return "jar". equals(person.getName());

11
_INTER_•

Also called Yoda conditions.

17
thephotoman•

I managed to get Yoda conditions written into my project's style guide.

3
tkrengel•

Awesome website! lmao XD

2
chris13524•

Only if you want to return in either case. There could be more statements after the if

2
A_random_zy•

yea that could be true tho

1
Fiskepudding•

As for lower/uppercasing before comparing, be aware of the Turkish and Azerbaijani dotted and dotless i.

5
Professor_Dr_Dr•

What do you need to be aware of? That those might look weird/wrong when testing?

2
Fiskepudding•

That the lowercasing of a capital dotless I in Turkish locale turns into a lowercase dotless i for example. So MILES lowercase is not miles. And a lowercase miles uppercase in Turkish locale is not MILES.

There are probably some blog posts out there explaining this better than me.

I believe I read somewhere (possibly by Microsoft) that uppercasing when running in English locale is the safest.

6
knoam•

The correct thing to use is .equalsIgnoreCase(). Whether uppercasing or lowercasing is better depends on the language and probably in some cases the specific characters. The German ß lowercases to being split into two characters: ss. So there's a concept called "fold case" which means the correct case to use for comparison and it will be upper or lower case depending on the specific character.

I learned this from this super interesting talk

https://youtu.be/TmTeXcEixEg

It starts with a lot of Perl 5 specific stuff but about halfway through it pivots to more Unicode focus.

10
mutleybg•

Objects.equals()

Very nice tip, I didn't know Objects.equals() 👍

2
talking_window•

I would rather recommend using StringUtils from the Apache Commons.

-11
_jetrun•

I would rather recommend using StringUtils from the Apache Commons.

Here's the source for Objects.equals:

public static boolean equals(Object a, Object b) {
    return (a == b) || (a != null && a.equals(b));
}

Here's the source for org.apache.commons.lang.StringUtils.equals

public static boolean equals(String str1, String str2) {
    return str1 == null ? str2 == null : str1.equals(str2);
}

Why would you recommend StringUtils from Apache Commons?

10
TheBonafideBoy•

There is an assignment to something that is not a variable.

31
apetersson•

this should be the top comment, it won't even compile for that reason.

return "jan".equals(person.getName())

seems like a reaasonable answer

14
fromtheheap•

You don't know what to do when it is "false", just guess it.

6
[deleted]•

[deleted]

1
thephotoman•

s/equals/equalsIgnoreCase/g

0
substitute-bot•

this should be the top comment, it won't even compile for that reason.

return "jan".**equalsIgnoreCase**(person.getName())

seems like a reaasonable answer

^^This ^^was ^^posted ^^by ^^a ^^bot. ^^Source

-1
MrDOS•
“jan”

Those curly quotes aren't going to compile 😉

22
giorgiga•

Comments on your points:

  1. That might actually be ok: it is customary to write comparisons so that NPEs are avoided as much as possible, but its's also true that NPEs are actually desirable in many cases (see `Objects.requireNonNull()`) and, in modern code, most variables are guaranteed/validated as not null anyways
  2. On top of that you say, the code won't even compile (which, in a sense, is the only thing undebatably wrong about it)
  3. Good point – I didn't think of that! (but then in my defense it's not like the scenario of comparing person names with a constant is even remotely realistic... common sense is optional in fantasy scenarios)
  4. No design/style can be seen there: it's a just stupid code from an artificial interview question (BTW there is no "design" that can be shown in 1 line of code: maybe you can call of "coding style", but certainly not of design)
16
illhxc9•

OP is correct that this won't compile. The result of the "=" operator is the object that is assigned so that will be a string and not a boolean. The if statement requires a boolean to compile though. Also you can't assign to a non-variable.

9
giorgiga•

I didn't check this, but I guess compilation will actually fail because the code tries to assign to a function call and not on type concerns.

5
illhxc9•

Yeah I think you're correct.

2
BestUsernameLeft•

I might be misunderstanding you, but I want to clarify: it tries to assign "jan" to the value returned from the method call, which (presumably) will be a string.

0
giorgiga•

Not really: the point is you can only assign to variables: assigning to a function call (or a literal value) is not allowed regardless of their type.

The following, using booleans instead of strings:

public class X {
	public static void main(String[] args) {
		if ("".isEmpty() = true) {
			System.out.println("yep");
		} else {
			System.out.println("nope");
		}
	}
}

fails to compile for the same reason:

$ javac X.java 
X.java:3: error: unexpected type
		if ("".isEmpty() = true) {
		              ^
  required: variable
  found:    value
1 error
3
dimensions1210•

I'd probably say that rather than having a return true with no return false branch it would be better to have

return (person.getName().equalsIgnoreCase("jan"))

Otherwise if the person is not Jan the method (assuming a method since there is a return statement) will not return anything (assuming there is no other code)

12
Alxe•

You can go full-Yoda and say "jan".equalsIgnoreCase(person.getName()) and return false when comparing on null.

27
gle42•

Definitely not the same thing. In the original code, the intention is to not return if the name is not jan but continue execution.

Here you always return, with true or false value.

11
dimensions1210•

Yes. My point was that, looking at the code without the context, it could be an error since, if you put that in a method without anything else, you will return true if the name is "jan" and nothing otherwise.

Hence why I said " Otherwise if the person is not Jan the method (assuming a method since there is a return statement) will not return anything (assuming there is no other code)"

i.e. if this is the only code and that code is in a method, it is probably wrong

3
thephotoman•

You've still got the NPE issue. Yoda it.

1
vortexrikes•

What about the fact that it won't compile?

9
RapunzelLooksNice•

Just a detail... ;)

2
hugthemachines•

That's just, like, the compiler's opinion, man.

2
victor-martinez-roig•

moreover Jan is not configurable, I would rahter use some properties file or DB where I could change this name

6
RapunzelLooksNice•

Nah, it’s Java. You should have a bunch of abstract factories and some dependency injection to look more pr0 /s

2†
knoam•

Dependency injection could be pulling the value from a config file or the database. You never know, which is the point.

2
i_donno•

Yeah, and it looks like a month so maybe its should be getMonth()

0
DJDavio•

Adding to the other valid comments: "jan" looks like a first name, but what does person.getName() likely return? Maybe a last name, maybe a complete name, but usually not a first name.

6
rubyrt•

If person.getName() returns null you'll also get a NPE. That can be mitigated by reversing to "jan".equals(person.getName()) (as long as person is not null, of course).

I think the main point they are getting at though is that = is the wrong comparison operator.

PS: if we want to get picky, the curly brackets are superfluous.

6
springuniOP•

If person.getName() returns null you'll also get a NPE.

if(person.getName() == “jan”) {return true;}

This is not so. person.getName() == “jan” just evaluates to false.

3
rubyrt•

Sorry for my sloppyness, I had replaced = or == by .equals() without mentioning it. You can infer it from me using "jan".equals(person.getName()) which avoids the potential NPE in person.getName(),equals("jan").

1
Wobblycogs•

Unless the persons name is "jan" and it had been intern'ed in which case it would return true as the literal "jan" would match intern value making the VM use the same object for both. I would not recommend using intern though, it can lead to a world of hurt.

1
Trailsey•

I'd add that "jan" is a literal with no context. It should probably be externalized from the method, in a constant or maybe from config.

5
bhaskarsingh007•

In code we are trying to assign value to a method which will not work.

4
springuniOP•

Though I noted ‘=‘ is wrong, I can see now from the comments that I should have explicitly stated that this makes the code not to compile.

3
barnuska123•

To be pedantic it's not trying to assign anything to a method. In java methods are not first class citizens, so even if the code wrote person::getName =... would not try to actually assign to the method itself. It's evaluated eagerly and would want to assign one reference (ref to "Jan") to another (ref that get Name returns). Java doesn't have an explicit definition of rvalue/lvalue (and others) like c++ does but that's what could be used here to most accurately describe why it's not allowed. getName returns a temporary reference that's an rvalue (can only be in the right hand side of an assignment) in C++ terms.

1
[deleted]•

This is just an optimization but the if is redundant, you can just do 'return x.equals(y)'

3
victor-martinez-roig•

no you cant as you do not know what do to when it is false

6
[deleted]•

This is true, the question is a bit ambiguous in that regard but if it said return false otherwise then you could say to do that as best practice

3
leonbadam•

You can just return the result of the comparison instead of true separately

3
SquidgyTheWhale•

That changes the logic. The original code doesn't return if false.

3
leonbadam•

If you return true you're probably using it as a logic value so having false would also work

0
syn_ack•

Why's "jan" special?

I'd prefer to see this in a documented constant. If there are multiple special people for the same reason, then it may be better have a list of names to check against. If it's because they're an admin (or some other style of role) then this should be checked in another way (`person.hasAccess(Level.ADMIN)` -- for example).

3
Juzzz•

I'd prefer to see this in a configuration. What if the value is different per environment.

1
SnowPenguin_•

I don't think you missed anything really. To avoid the NullPointerException, one thing you could do is to rewrite the condition as follow:-

if(“jan”.equal(person.getName()) ) {return true;}

Since equal() doesn't mind if you gave it null for a value.

3
Juzzz•

An exception is still thrown when 'person' is null ^^

2
SnowPenguin_•

You're right! But it works when person isn't null but the name is.

1
Juzzz•

Thats true.

I mean that results in false.

3
EpicOweo•

I'm taking a shit while using reddit and I almost just laughed. That would have been awkward

2
atehrani•

The if return is not needed it can be refactored as such

return person != null && "jan".equals(person.getName());

3
persicsb•

The foremost problem is, that this code does not compile.

2
springuniOP•

Yes, I think I should have been more clear about that ‘=‘ at that place means that the code actually doesn’t compile.

2
Waksu•

Aside form everything else rest of the folks said.

This code does not convey any meaning as why are we checking if person name is jan, we lose business context. Also this using raw, "meaningless" getters isn't domain and encapsulation friendly, it would be much better if we could abstract this comparison behind some well though abstraction.

For example

person.isWellKnownClient()

Would give us much more business context.

2
keepcoderpro•

I would also clearly clarify the question, what do they want in general?

2
springuniOP•

Good point, but the question was asked offline as part of the application form. I had no point of contact to whom I could ask clarifying questions.

1
audioen•

Also "if" is not a function, but a keyword. I believe whitespace is appropriate before the parenthesis, as would be adding newlines after brace, and some indentation. This can be you just editorializing the code, though.

Some people dislike multiple returns from a function, and the lack of structure implied by an "if (...) { return ...; } ...something else here, then return..." style of programming. Possibly code could be written in this way, adding an else-block:

public boolean foo() {
    if (...) {
        return true;
    } else {
        // N.B. I am assuming the stuff here isn't just "return false"
        ...
        return something;
    }
    // no return here, all if blocks must explicitly return
}

It can feel like unnecessary pedantry, but this kind of style honors structural programming's promise that control flow can be analyzed without looking into the contents of the {} block, e.g. compiler proves for you that "if" selects either block for execution, as the code pattern is a classic "if-else", and the lack of return at end guarantees that all code flows will terminate within either if branch. In general, code editing can be more robust if valid control flow contains assumptions that the compiler can validate. E.g. try to assign to variables just once, so that they become effectively final and may be uninitialized if you have a bug somewhere, and so on.

1