[xgb84j]

Great idea, that's exactly what I like to do as well.

Because many people were interested in some actual code, here is an example that we used for Java:

---

public class UserDao {

    private Provider<Session> sessionProvider;

    public UserDao() {
        this.sessionProvider = new DbSessionProvider();
    }

    public boolean saveUser(User user, ApiConfig config) {
        try {
            Session session = sessionProvider.get();
            session.save(user);
            System.out.println("User saved!");
            return true;
        } catch (DbException e) {
            EmailUtil.sendEmail(config.getTechnicalSupportEmail(), e);
            return false;
        }
    }

}

---

"Solution": We always ask the candidate "What would you change about this code?" or something similar. We expect the candidate to come up with some selection of: - Database sessions should come an injected provider. - Configuration should probably be injected as well and not passed as a method parameter. - Booleans are not canonical Java as return value. - Don't write to stdout but to a logger. - Exception handling should probably happen outside the DAO. - Don't use static methods for sending Emails without good reason. Non-static methods are easier to test.

Finding these things is as important as being able to talk about them and give background on advantages / disadvantages of doing things different ways. With good candidates one can talk easily half an hour just about this example and adjacent topics (e.g. error handling).

---

Edit: Formatting

Edit 2: added "Solution"

[normie3000]

This code looks like normal enterprise Java ugliness to me - I object to it on principle, but apart from having to pass ApiConfig when saving a user I probably wouldn't identify any of the official problems you listed without familiarity with the rest of the codebase.

[Izkata]

Same here... and funny enough the only thing that really looks wrong to me is they used "this" in only one of the two places they used "sessionProvider".

It's not a bug ("this" is implied, so it works just fine), but leaving it out hints to me that it was originally a third argument to "saveUser()", and either the original person writing this didn't have a solid API in mind or "saveUser()" was intended to also be static like "EmailUtil.sendEmail()" and it was switched around later on. Overall hints towards two conflicting styles in the same codebase and no good guidelines. Or maybe there are such guidelines (as hinted in the "solution"), and whoever updated this class only did it partway. This is the point where I'd poke around in the repo history to see why it's like this and whether anything should be changed further in either direction.

[jerf]

The "secret decoder ring" for this sort of question is that it's really "say something intelligent about this code that indicates you have real and good experience" even more so than "find the bug". So your answer would generally be off to a good start as well.

Given the Java code in question, while I understand the idea that "well, the session provider may just be hard coded" and such, I would definitely want to hear something about deficient exception handling. It's obviously an important part of the code and while I may not be able to spew out the exact correct exception handling without knowing more about the context and what exceptions may occur, it is obviously very wrong as written.

[bilekas]

I love the exception suppression via email.

Sr.Dev : "UserSignup failed, ask tech support what happened in our code"

Genius.

[actionfromafar]

This is canonical Java. Commit and deploy!

[vaylian]

> Booleans are not canonical Java as return value

What should you return instead? Or should the method return void and raise an exception on failure?

[haspok]

You could return the saved User object, if `save` changes it in any way, to allow the caller to work with it functional style, ie. make it explicit that it is an updated object (or if it is immutable, although typical persistence frameworks expect mutable objects, "bean" style).

You could also go fancy and do it properly functional, so return something like an `Either<User, Error>` instead of throwing an Exception, but that's definitely not canonical Java...

[normie3000]

Probably boolean. If you don't see the difference, welcome to Java :D

[Izkata]

Ah, but the code already uses boolean instead of Boolean.

[normie3000]

Whoops, so it does. In which case I have no idea what the solution is referring to.

[xgb84j]

No serious Java code uses booleans to indicate failure or success. It's on the same level as not using a language's standard variable naming scheme. It's not wrong, but something experienced Java developers working on good code will immediately notice.

In good Java code you would return either void to indicate side effects or the newly saved user. Exception handling should be done outside the function, because depending on the context you might retry etc.

The main idea is to check if people who say they are very experienced understand conventions and have experience with common Java libraries.

[hkon]

Ever get the answer: this code should not exist?

[whatever1]

"Don't write to stdout but to a logger."

Not great advice given the fact that a logger paged pretty much every SDE on this planet.

[nailer]

That's the intention. So people see the messages.

That said, modern Linux OSs send stdout to journald by default. journald should forward to some centralized logging server.

[whatever1]

It's an extra dependency. But I guess we will never learn. Just import log4j and pray.