[GitHub] [cloudstack] rafaelweingartner commented on a change in pull request #4256: Fix comparison using nullable objects

[GitBox <gi...@apache.org>]

rafaelweingartner commented on a change in pull request #4256:
URL: https://github.com/apache/cloudstack/pull/4256#discussion_r468162010



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -2199,7 +2199,7 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
         // OfflineVmwareMigration: check storage tags on disk(offering)s in comparison to destination storage pool
         // OfflineVmwareMigration: if no match return a proper error now
         DiskOfferingVO diskOffering = _diskOfferingDao.findById(vol.getDiskOfferingId());
-        if (diskOffering.equals(null)) {
+        if (diskOffering == null) {

Review comment:
       This is the only one that makes sense to change.

##########
File path: utils/src/test/java/com/cloud/utils/TernaryTest.java
##########
@@ -28,7 +28,7 @@ public void testEquals() {
         Assert.assertEquals(new Ternary<String, String, String>("a", "b", "c"), new Ternary<String, String, String>("a", "b", "c"));
         Assert.assertFalse(new Ternary<String, String, String>("a", "b", "c").equals(new Ternary<String, String, String>("a", "b", "d")));
         Assert.assertFalse(new Ternary<String, String, String>("a", "b", "c").equals(""));
-        Assert.assertFalse(new Ternary<String, String, String>("a", "b", "c").equals(null));

Review comment:
       You do not need to remove the `equals` here. In this very specific use case, there is no null pointer that can happen here.

##########
File path: engine/orchestration/src/test/java/com/cloud/agent/manager/ConnectedAgentAttacheTest.java
##########
@@ -44,7 +44,7 @@ public void testEqualsFalseNull() throws Exception {
 
         ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link, false);
 
-        assertFalse(agentAttache1.equals(null));

Review comment:
       The same here. I do not understand why people coded this check, but it does not cause a null pointer. Moreover, if you change the equals to `==` here, you are changing the test case.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org