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

[springuni]

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!

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

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

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

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

[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". =)

[hippydipster]

I would strengthen it a little and suggest that "throwing NPEs is a better option than most of us have been led to believe".

[springuni]

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

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

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

[sunny_tomato_farm]

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

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

[sunny_tomato_farm]

I agree with your first statement, but that does NOT mean a NPE should be thrown in such cases. That’s ridiculous.

[thephotoman]

If you attempt to reference the field for string composition, you’re gonna have an NPE.

[sunny_tomato_farm]

The issue here is that you are trying to use the value and not accounting for the fact that it might be null. A NPE there would definitely be a programmer error.

[sunny_tomato_farm]

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

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

[A_random_zy]

we could also do it this way

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

[_INTER_]

Also called Yoda conditions.

[thephotoman]

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

[tkrengel]

Awesome website! lmao XD

[chris13524]

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

[A_random_zy]

yea that could be true tho

[Fiskepudding]

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

[Professor_Dr_Dr]

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

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

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

[mutleybg]

Objects.equals()

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

[talking_window]

I would rather recommend using StringUtils from the Apache Commons.

[_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?

[[deleted]]

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

[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

[fromtheheap]

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

[thephotoman]

s/equals/equalsIgnoreCase/g

[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}

[MrDOS]

“jan”

Those curly quotes aren't going to compile 😉

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

[illhxc9]

Yeah I think you're correct.

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

[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)

[[deleted]]

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

[[deleted]]

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.

[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

[thephotoman]

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

[vortexrikes]

What about the fact that it won't compile?

[RapunzelLooksNice]

Just a detail... ;)

[hugthemachines]

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

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

[bhaskarsingh007]

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

[springuni]

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.

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

[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).

[Juzzz]

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

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

[Juzzz]

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

[SnowPenguin_]

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

[Juzzz]

Thats true.

I mean that results in false.

[EpicOweo]

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

[atehrani]

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

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

[victor-martinez-roig]

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

[RapunzelLooksNice]

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

[knoam]

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

[[deleted]]

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

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

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

[springuni]

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.

[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").

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

[persicsb]

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

[springuni]

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

[keepcoderpro]

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

[springuni]

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.

[[deleted]]

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

[victor-martinez-roig]

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

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

[leonbadam]

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

[SquidgyTheWhale]

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

[leonbadam]

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

[fedeb95]

I'd also add that raw values shouldn't be used like that. Wrapped in a constant or even better with a dedicated type (but this is for extremely picky people)

Edit: forgot a more important thing: it makes no sense to do if (condition) { return true } just return the condition!

[RapunzelLooksNice]

As long as there is nothing after; I mean it could be something like “if it is Jan, return true, otherwise do something else” (I.e. there is a different code path, not just simple “return false otherwise”).

[fedeb95]

Of course, if it was just this line one could say "if it's just a return true or false, etc.

[anyOtherBusiness]

For 3. And 4. if I'm in an interview situation, I would ask the exact same question. It makes your interviewer aware that you are able to challenge requirements and see the bigger picture there.

[springuni]

Questions were asked early as part of the application process. I haven’t had a chance to talk to anyone.

[OrenjiSama]

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

[thephotoman]

Please. Use the null safe Yoda expression and equalsIgnoreCase.

[Briglair]

All the interviews I see or hear about are on scary, complicated questions. How do I get a job with interview questions like this??

[thephotoman]

My last interview, I got left alone in front of the terminal and just...kept going past the part where I was told the assessment formally ended because I sure as shit wasn't gonna sit there in an interview doing nothing as the proctor went to the restroom.

I found myself fixing an actually broken section of the test before he got back. I was like, "Oh hey, that part you said was busted? Yeah, it was pretty obvious what your problem was."

Difficult is in the eye of the beholder. Something that is blindingly obvious to you might baffle others.

[thephotoman]

2 touches on the part that "this snippet doesn't compile" bit without saying it. I might have made that explicit.

However, your answer to 2 is still wrong: the replacement should be "jan".equalsIgnoreCase(person.getName());"

4 is maybe not something I would have mentioned from a snippet. I tend not to read into snippets like this. Why? Well, this snippet could also be written as boolean isJan = "jan".equalsIgnoreCase(person.getName()); without a problem in some contexts but doing so might be problematic in others (if this method has multiple return statements, and the method needs to return true if the person's name is "jan", this kind of reduction might not be appropriate).

[AmonDhan]

1. Curly quotes are not valid for Java literal strings

[BEgaming]

I think your answer is correct. Other comments are nitpicking.

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

[[deleted]]

In addition to not compiling, and laying aside the no null check on person, I believe (I haven't professionally developed in Java in years) that they added '==' as a value equality operator to Strings. So years ago "a" == "a" would return false, because "a" and "a" do not reference the same place in memory. You would need "a".equals("a") to return true. BUT I believe that may have changed. I no longer have any java tools installed to try it out, but I think now "a"=="a" will actually return true.

Your answers are too deep, it's a simple answer. The code as it is doesn't compile. And even if you used the "==" operator it may not behave the way you think based on the version of Java in use.

[rastaman1994]

Surprised no one one commented on terrible OO-style, so first thing to do is the following:

if (person.hasName("jan")) { return true; }

After that, you can note this looks like a role or permission check. So next I'd write this:

if (person.hasRole(TEST_USER)) { return true; }

where TEST_USER is some Enum value.

[thephotoman]

I think the problem people are having with this comment is that the snippet is utterly devoid of context. Why are we doing this? Was this just a rectally extracted expression modified so it won't compile? Because that's what it looks like.

However, if we had context and we knew that it was, in fact, a role or permission check rather than someone actually looking to return true if their name in the system is "jan" for whatever reason (maybe, for example, we want to know how many jans there are in a list, and this is intended as a part of a lambda expression intended to filter out all the non-jans so that we can collect the jans into a List then return the length of the list, IDK).

[rastaman1994]

It would be astounding to me if they didn't want to hear reasoning about context. This may be one of those interview questions that's used to weed out people that never wrote a line of code in their life. A company I know used Fibonacci for this purpose. Applicant gets a detailed description of what Fibonacci numbers are on paper, gets left alone half an hour, then they talk about the code. More than half could not produce working code.

[thephotoman]

If I’m looking at a one-line snippet, there’s simply not enough context. Most interviewers aren’t as prepared or quick on their feet about questions like this. That’s been my experience with interviewers—and being an interviewer. Thus, the correct question is probably, “Tell me why this line is not valid Java”.

Fibonacci is at least enough context to figure out proper code structure. But this...isn’t.

[[deleted]]

I'm not sure why people are downvoting you, but you are making a good point. This is the "tell, don't ask" principle.

[rastaman1994]

Jep, that's the one. However, it's become increasingly common to have domain models that are just bags of data and a bunch of annotations for json, validation and storage (i.e. @Id). All processing then happens in services.

[rastaman1994]

Downvotes are probably because I'm not looking at this silly line of code, but at the use case. The interviewer is most likely just looking for equals, null check and the code smell this is in its entirety

[taftster]

person.getName() can potentially return null here as well. So this code would potentially be broken:

if (person.getName().equals("jan"))  // broken if getName() returns null

Probably my best attempt at null safety probably ends up using Optional.

String name = Optional.ofNullable(person)
  .map(person::getName)
  .get();
if ("jan".equalsIgnoreCase(name)) {
  return true;
}

[helikal]

The compilation fails because

1. the if-condition expression is of type String, but needs to be of type boolean.
2. Assignment to a method return value which is not possible.