CPS Code Review Check List

Optional<String> optionalResponseBody = Optional.ofNullable(responseEntity.getBody())
.filter(Predicate.not(String::isBlank));
return (optionalResponseBody.isPresent()) ? convert(optionalResponseBody.get())
: Collections.emptyList();

String responseBody = responseEntity.getBody();
if (responseBody == null || responseBody.isBlank()) {
    return Collections.emptyList();
}
return convert(responseBody);

Don't forget to check the copyright
(add/modify new calendar year to existing copyright if needed)

*  Copyright (C) 2025 Other Company

*  Copyright (C) 2021 Other Company
*  Modifications Copyright (C) 2025 OpenInfra Foundation Europe.

Don't forget to check the commit message structure

Follow the ONAP commit message guidelines

Consistent naming
Note. There will be conflicts between names in 3PP packages or even our on modules. In that case use the correct name according to our codes context as soon a you can.
Note. It is possible that a variable changes name because of the context of the class/method. Especially when calling a common utility method

int job = 123;
String response = getStatus(job);

String getStatus(String orderNumber) { .... };

int orderNumber = 123;
String status = getStatus(orderNumber);

String getStatus(int orderNumber) { .... };

Don't shorten instance names

CmHandleState state = new CmHandleState(..);

CmHandleState cmHandleState = new CmHandleState(..);

Overuse of constant for string literals (that don't add value)
Also duplication is not a good reason, it often 'smells like' a method that should be extracted out instead

final static String HELLO = "Hello ";
String message = HELLO + name;

String message = "Hello " + name;

String sayHello(final String name) {
  return "Hello " + name;
}

When using equals() to compare a value with a constant put the constant first to prevent NPEs

if (xpath.equals(ROOT_NODE_PATH) {..}

if (ROOT_NODE_PATH.equals(xpath) {..}

Avoid using ! when else block is implemented 

if (x!=null) {
  do something;
} else {
  report error;
}

if (x==null) {
   report error;
} else {
   do something;
}

No need for else after return statement

if (ROOT_NODE_PATH.equals(xpath) {
   return something;
} else {
   return somethingElse;
}

if (x==true) {
  return something;
}
return something-else;

No need to check for not empty before iterating on collection

if (!myCollection.isEmpty()) {
  collection.forEach(some action);
}

collection.forEach(some action);

Use String concatenation instead of String.format (5x slower!) where possible 

String.format("cm-handle:%s",cmHandleId);

"cm-handle:" + cmHandleId;

Initialize collections/arrays with known size where possible

void processNames(Collection<String> orginalNames)  {
   String[] processedNames = new String[0];

void processNames(Collection<String> orginalNames)  {
   String[] processedNames = new String[orginalNames.size()];

Use @Service (org.springframework.stereotype) when a class includes operations.
Use @Component when it is a data object only.
Currently there is no functional difference in Spring but this might change.

@Component
public class TrustLevelManager {
     void handleInitialRegistration(...)
        { ... };
}

@Service
public class TrustLevelManager {
     void handleInitialRegistration(...)
        { ... };
}

Use common names for objectUnderTest and result

def parameterMapper = new ParameterMapper()
when:'the request parameters are extracted'
def parameters = parameterMapper.extractRequestParameters(..)

def objectUnderTest = new ParameterMapper()
when:'the request parameters are extracted'
def result = objectUnderTest.extractRequestParameters(..)

Slogans (test name) should NOT contain expectations.
Use then: block instead

def 'Registration of invalid cm handle throws exception.'() {
  ...}

def 'Registration with invalid cm handle name.'() {
  ...
then: 'a validation exception is thrown'
... }

Descriptions missing or are not up to date

given: 'a cm handle'
  cmHandle.id = 'invalid,name'

given: 'a invalid cm handle name'
  cmHandle.id = 'invalid,name'

No unnecessary test data Try to minimize the test data to just the data needed for the use case under test. Often test data from other test is copied and pasted into new test that simply don't need it.
Tip: Use Groovy Map Constructor to only populate fields required for test

given: 'invalid cm handle name'
  cmHandle.id = 'invalid,name'
  cmHandle.dmiProperties = [dmiProp1: 'dmiValue1']
when: ...

given: 'invalid cm handle name'
  cmHandle = new CmHandle(id:'invalid,name')
when: ...

Clearly distinguish between important values and values that do not affect the test.
Tip. use ‘my’ or ‘some’ prefixes for strings or variable names
(don't be afraid to use spaces for readability if valid for the test)

when: ‘permission is checked for unauthorized user‘
objectUnderTest.checkPermission(cmh1, ‘user’,’scope')
then: ‘exception contains the user id’
thrown(PermissionException).message.contains(’user')

when: ‘permission is checked for unauthorized user‘
objectUnderTest.checkPermission(someCmh, ‘my user’,’some scope')
then: ‘exception message contains the user id’
thrown(PermissionException).message.contains(’my user')

Don't use java when not needed, like

• types

• access modifiers

• ;

• “ (only needed for GString or when mixing “ and ')

Note. static is acceptable when needed as it is a shorter and more intuitive equivalent of @Shared

private CmHandle myCmHandle
= new CmHandle("my cm handle","",null);

def myCmHandle = new CmHandle(id: 'my cm handle')

Do not log any user data at any log level

Since we do not know what is in the user data there could also be sensitive information inside it.
Be aware of logging objects, make sure the toString() implementation doesn't include user data for that object. So instead maybe just log fields that are well defined and do not contain user data.