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

quarks
  • 33,478
  • 73
  • 290
  • 513
  • What did you mean by *is there a better way to represent this code with the use of != null for instance:* ? – Youcef LAIDANI Oct 20 '20 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 Oct 20 '20 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 Oct 20 '20 at 16:50
  • also I prefer to avoid returning null on methods, basically to make the client code cleaner, check this interesting post https://stackoverflow.com/questions/1274792/is-returning-null-bad-design – Carlos Oct 20 '20 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 – Youcef LAIDANI Oct 20 '20 at 17:18
  • @YCF_L does it look like RxJava mess? – quarks Oct 20 '20 at 18:00
  • @quarks I don't get you, but what I mean is to create `RemoteEntity entity = new RemoteEntityBuilder() .appId(appId) .nameSpace(nameSpace) .entityType(entityType) .entityId(entityId) .blobs(Lists.list(new RemoteBlobBuilder() .blobName(blobName) .blobStream(new SimpleRemoteInputStream(inputStream)) .build())) .build()` and then `entityRepository.saveEntity(entity)` this can be more clear – Youcef LAIDANI Oct 20 '20 at 18:02
  • @YCF_L I updated the question with the actual code in the IDE, do you think the code is not clean? – quarks Oct 20 '20 at 18:15
  • No, no, I mean, just to use something like this https://ideone.com/Y4eVlg – Youcef LAIDANI Oct 20 '20 at 18:21

2 Answers2

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(() -> ..)
Youcef LAIDANI
  • 55,661
  • 15
  • 90
  • 140
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) ...
Dave Costa
  • 47,262
  • 8
  • 56
  • 72