1

I have a lot of this kind of code in my project:

if (entityRepository.saveEntity(new RemoteEntityBuilder()
   .appId(appId)
   .nameSpace(nameSpace)
   .entityType(entityType)
   .entityId(entityId)
   .blobs(Lists.list(new RemoteBlobBuilder()
      .blobName(blobName)
      .blobStream(new SimpleRemoteInputStream(inputStream))
      .build()))
   .build()) != null) {  
   // Meaning entity was saved
} else {
   // Meaning entity was not saved
}

The saveEntity method returns either NULL (if operation failed) or the object/entity that was saved if the operation was successful. My question is, is there a better way to represent this code with the use of != null for instance:

if(entityRepository.saveEntity(...)) {
}

Or something else.

UPDATE:

The saveEntity method is this

  @Override public RemoteEntity saveEntity(RemoteEntity entity)
      throws NotBoundException, RemoteException {
    RemoteEntities remoteEntities = saveEntities(new RemoteEntity[] {entity});
    return remoteEntities != null ? remoteEntities.entities().stream().findFirst().get() : null;
  }

Here's how it looks now thanks to YCF_L:

  entityRepository.saveEntity(new RemoteEntityBuilder()
                .appId(appId)
                .nameSpace(nameSpace)
                .entityType(entityType)
                .entityId(entityId)
                .blobs(Lists.list(new RemoteBlobBuilder()
                    .blobName(blobName)
                    .blobStream(new SimpleRemoteInputStream(inputStream))
                    .build()))
                .build()).ifPresentOrElse(remoteEntity -> {
              pubSubService.updated(remoteEntity.appId(), remoteEntity.nameSpace(),
                  remoteEntity.entityType(), remoteEntity.entityId());
              setStatus(Status.SUCCESS_CREATED);
            }, () -> {
              setStatus(Status.CLIENT_ERROR_BAD_REQUEST);
            });

Here's how the code looks in the IDE (looks pretty clean to me):

enter image description here

9
  • What did you mean by is there a better way to represent this code with the use of != null for instance: ? Commented Oct 20, 2020 at 16:47
  • I mean I can build a new method like saveEntityBoolean that wraps this but that's a bit an overkill and API looks terrible. Something like, if(saved) { // do stuff }
    – quarks
    Commented Oct 20, 2020 at 16:49
  • 3
    using exceptions properly might be an approach.. and you avoid the usage of null.. this method "saveEntities" probably should throw some exception if it not succeeds saving the entity
    – Carlos
    Commented Oct 20, 2020 at 16:50
  • also I prefer to avoid returning null on methods, basically to make the client code cleaner, check this interesting post stackoverflow.com/questions/1274792/…
    – Carlos
    Commented Oct 20, 2020 at 16:55
  • @quarks I think it is ugly the way you are building your object, I would create a separate variable, this can make your code more clear and clean Commented Oct 20, 2020 at 17:18

2 Answers 2

4

I would use Optional in your case :

public Optional<RemoteEntity> saveEntity(RemoteEntity entity) throws NotBoundException, RemoteException {
    RemoteEntities remoteEntities = saveEntities(new RemoteEntity[]{entity});
    return remoteEntities.entities().stream()
            .findFirst();
}

and then :

if(entityRepository.saveEntity(...).isPresent()) {
   ...       
}

In fact you have many choices with Optional, you can use ifPresent also :

entityRepository.saveEntity(...)
    .ifPresent(r -> ..)

Or throw an exception:

entityRepository.saveEntity(...)
    .orElseThrow(() -> ..)
2
  • Yeah, this actually makes sense
    – quarks
    Commented Oct 20, 2020 at 16:51
  • 1
    stream().findFirst() actually returns an Optional. You don't need .orElse(Optional.empty())
    – sapisch
    Commented Oct 20, 2020 at 16:54
1

What is "better" may be a matter of opinion.

Given your example, the way to achieve that would be to create another method that calls saveEntity() and returns true or false. (I do wonder why saveEntity() doesn't throw an exception if its operations fails -- that would be more normal in my experience.)

If you simply don't like that the comparison is hard to spot, you might reverse the order:

if (null != entityRepository.saveEntity(...))

I would probably move the call outside of the if entirely, as I find side effects in conditionals potentially confusing.

RemoteEntity myEntity = entityRepository.saveEntity(...)
if (myEntity != null) ...

Not the answer you're looking for? Browse other questions tagged or ask your own question.