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

[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

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