Skip to main content
Code Review

Return to Question

Rollback to Revision 1
Source Link
Heslacher
Heslacher
  • 51k
  • 5
  • 83
  • 177
@AllArgsConstructor
public class RegistrablePersonRegisteredPerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegistrablePersonRegisteredPerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegistrablePersonsRegisteredPersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegistrablePersonRegisteredPerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegistrablePersonRegisteredPerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegistrablePersonRegisteredPerson. For example, In a situation when the RegistrablePersonRegisteredPerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

public class RegistrablePersonRegisteredPerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegistrablePersonsRegisteredPersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegistrablePersonRegisteredPerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

// RegistrablePersonRegisteredPerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;

EDIT

According the Torben's comment changed RegisteredPerson => RegistrablePerson

@AllArgsConstructor
public class RegistrablePerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegistrablePerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegistrablePersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegistrablePerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegistrablePerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegistrablePerson. For example, In a situation when the RegistrablePerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

public class RegistrablePerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegistrablePersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegistrablePerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

// RegistrablePerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;

EDIT

According the Torben's comment changed RegisteredPerson => RegistrablePerson

@AllArgsConstructor
public class RegisteredPerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegisteredPerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegisteredPersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegisteredPerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegisteredPerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegisteredPerson. For example, In a situation when the RegisteredPerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

public class RegisteredPerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegisteredPersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegisteredPerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

// RegisteredPerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;
RegisteredPerson => RegistrablePerson
Source Link
Barney
Barney
  • 121
  • 2
@AllArgsConstructor
public class RegisteredPersonRegistrablePerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegisteredPersonRegistrablePerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegisteredPersonsRegistrablePersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegisteredPersonRegistrablePerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegisteredPersonRegistrablePerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegisteredPersonRegistrablePerson. For example, In a situation when the RegisteredPersonRegistrablePerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

public class RegisteredPersonRegistrablePerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegisteredPersonsRegistrablePersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegisteredPersonRegistrablePerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

// RegisteredPersonRegistrablePerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;

EDIT

According the Torben's comment changed RegisteredPerson => RegistrablePerson

@AllArgsConstructor
public class RegisteredPerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegisteredPerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegisteredPersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegisteredPerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegisteredPerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegisteredPerson. For example, In a situation when the RegisteredPerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

public class RegisteredPerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegisteredPersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegisteredPerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

// RegisteredPerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;

@AllArgsConstructor
public class RegistrablePerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegistrablePerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegistrablePersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegistrablePerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegistrablePerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegistrablePerson. For example, In a situation when the RegistrablePerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

public class RegistrablePerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegistrablePersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegistrablePerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

// RegistrablePerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;

EDIT

According the Torben's comment changed RegisteredPerson => RegistrablePerson

Source Link
Barney
Barney
  • 121
  • 2

OOP at the boundary

I'm trying to implement a simple service in the true OOP way. This means for me that the domain objects has no technical or non-business-related methods (in practice no getters/setters).

Here are the domain interfaces to implement:

public interface Person {
    void register();
    void changeName(String name);
}

public interface Persons {
    Person register(String name);
    Person findByName(String name);
}

Solution #1

@AllArgsConstructor
public class RegisteredPerson implements Person {
    private Long id;
    private String name;

    private final PersonRegistry registry;

    public RegisteredPerson(String name, PersonRegistry registry) { 
        this.name = name; 
        this.registry = registry;
    }

    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = registry.register(name);
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            registry.changeName(id, name);
        }
    }
}

@RequiredArgsConstructor
public class RegisteredPersons implements Persons {    
    private final PersonRegistry registry;

    public Person register(String name) {
        if (registry.byName(name).isPresent()) {
            throw new PersonAlreadyRegisteredException(name);
        }
        Person person = new RegisteredPerson(name, registry);
        person.register();
        return person;
    }

    public Person findByName(String name) {
        return registry.byName(name)
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

I introduced a new non-public interface for a repository:

interface PersonRegistry {
    long register(String name);
    Optional<Person> byName(String name);
    void changeName(long id, String name);
}

class InMemoryPersonRegistry implements PersonRegistry {    
    private final Map<Long, PersonEntry> personEntries = new HashMap<>();
    private final AtomicLong idSequence = new AtomicLong();

    public long register(String name) {
        long id = idSequence.incrementAndGet();
        personEntries.put(id, new PersonEntry(id, name));
        return id;
    }

    public Optional<Person> byName(String name) {
        return personEntries.values().stream()
                .filter(entry -> name.equals(entry.name))
                .findAny()
                .map(entry -> new RegisteredPerson(
                    entry.id, entry.username, this
                ));
    }

    public void changeName(long id, String name) {
        if (personEntries.containsKey(id)) {
            personEntries.get(id).name = name;
        }
    }

    @AllArgsConstructor
    @EqualsAndHashCode(of = "id")
    private class PersonEntry {
        public long id;
        public String name;
    }
}

What I don't like about this solution it the coupling between InMemoryPersonRegistry and RegisteredPerson. For example, In a situation when the RegisteredPerson has more dependencies, all of them must be known to the InMemoryPersonRegistry. Possible solution for this would be to introduce a factory and make the repository (registry) dependent only on that factory. But the coupling still doesn't feel alright.

The repository is hard to test as well, because the returned object has no getters to prove correctness of the persistence.

Solution #2

In this version I implemented the registry as a dumb DAO with no business meaning, only as a tool to persistent entries. This breaks the coupling and makes the repository implementation independent of the other world.

interface PersonEntries {
    long save(PersonEntry entry);
    Optional<PersonEntry> byName(String name);
    void updateName(long id, String name);

    @RequiredArgsConstructor
    class PersonEntry {
        public final Long id;
        public final String name;
    }
}

On the other hand it's much more boiler-plate code on the client side:

public class RegisteredPerson implements Person {
    private final PersonEntries entries;
    /* ... */
    public void register() {
        if (id != null) {
            throw new PersonAlreadyRegisteredException(name);
        }
        id = entries.save(new PersonEntries.PersonEntry(null, name));
    }

    public void changeName(String name) {
        this.name = name;
        if (id != null) {
            entries.updateName(id, name);
        }
    }   
}

public class RegisteredPersons implements Persons {    
    private final PersonEntries entries;
    /* ... */

    public Person findByName(String name) {
        return entries.byName(name)
            .map(entry -> new RegisteredPerson(entry.id, entry.name, entries))
            .orElseThrow(() -> new PersonNotFoundException(name));
    }
}

When a Person structure changes, a lot of places in code must be modified.

Question is whether it makes sense to have the method updateName in the PersonEntries or would it be better to update everyting via save method.

Another point is the return value of the save, would it be better to update the entry and have void instead:

// RegisteredPerson.register():
PersonEntries.PersonEntry entry = new PersonEntries.PersonEntry(null, name);
entries.save(entry);
this.id = entry.id;

Any thoughts and ideas are appreciated! Thank you!