The intent here is to maintain a common style across the project and rely on the process to enforce it instead of individuals.
The pom.xml is configured to enforce a coding style defined in checkstyle.xml when
maven validate phase executed.
The project uses formatting rules described in .editorconfig file. Most of the popular IDEs have support of it. For
example, in IntelliJ IDEA hit Code -> Reformat Code to organize your code.
Line length is limited to 120 columns for *.java files. Other file types are unrestricted.
Don't use transitive dependencies in project code. If it needed, recommended adding a library as a dependency of Maven
in pom.xml directly.
It is recommended to define version of library to separate property in pom.xml:
<project>
<properties>
<caffeine.version>2.6.2</caffeine.version>
</properties>
<dependencies>
<dependency>
<groupId>com.github.ben-manes.caffeine</groupId>
<artifactId>caffeine</artifactId>
<version>${caffeine.version}</version>
</dependency>
</dependencies>
</project>
Do not use wildcard in imports because they hide what exactly is required by the class.
// bad
import java.util.*;
// good
import java.util.HashMap;
import java.util.Map;
Prefer to use camelCase naming convention for variables and methods.
// bad
String account_id = "id";
// good
String accountId = "id";
Name of variable should be self-explanatory:
// bad
String s = resolveParamA();
// good
String resolvedParamA = resolveParamA();
This helps other developers flesh your code out better without additional questions.
For Maps it is recommended to use To between key and value designation:
// bad
Map<Imp, ExtImp> map = getData();
// good
Map<Imp, ExtImp> impToExt = getData();
Make data transfer object(DTO) classes immutable with static constructor.
This can be achieved by using Lombok and @Value(staticConstructor="of"). When constructor uses multiple(more than 4) arguments, use builder instead(@Builder).
If dto must be modified somewhere, use builders annotation toBuilder=true parameter and rebuild instance by calling toBuilder() method.
// bad
public class MyDto {
private final String value;
public MyDto(String value) {
this.value = value;
}
public void setValue(String value) {
this.value = value;
}
public String getValue() {
return value;
}
}
// and later usage
final MyDto myDto = new MyDto("value");
myDto.setValue("newValue");
// good
@Builder(toBuilder=true)
@Value(staticConstructor="of")
public class MyDto {
String value;
}
// and later usage
final MyDto myDto = MyDto.of("value");
final MyDto updatedDto = myDto.toBuilder().value("newValue").build();
Although Java supports the var keyword at the time of writing this documentation, the maintainers have chosen not to utilize it within the PBS codebase.
Instead, write full variable type.
// bad
final var result = getResult();
// good
final Data result = getResult();
Enclosing parenthesis should be placed on expression end.
// bad
methodCall(
long list of arguments
);
// good
methodCall(
long list of arguments);
This also applies for nested expressions.
// bad
methodCall(
nestedCall(
long list of arguments
)
);
// good
methodCall(
nestedCall(
long list of arguments));
Please, place methods inside a class in call order.
// bad
public interface Test {
void a();
void b();
}
public class TestImpl implements Test {
@Override
public void a() {
c();
}
@Override
public void b() {
d();
}
private void d() {
...
}
private void c() {
...
}
}
// good
public interface Test {
void a();
void b();
}
public class TestImpl implements Test {
@Override
public void a() {
c();
}
private void c() {
...
}
@Override
public void b() {
d();
}
private void d() {
...
}
}
Explanation of an example:
Define interface first method, then all methods that it is calling, then second method of an interface and all methods that it is calling, and so on.
Not strict, but methods with long parameters list, that cannot be placed on single line, should add empty line before body definition.
// bad
public static void method(
parameters definitions) {
start of body definition
// good
public static void method(
parameters definitions) {
start of body definition
Use collection literals where it is possible to define and initialize collections.
// bad
final List<String> foo = new ArrayList();
foo.add("foo");
foo.add("bar");
// good
final List<String> foo = List.of("foo", "bar");
Also, use special methods of Collections class for empty or single-value one-line collection creation. This makes developer intention clear and code less error-prone.
// bad
return List.of();
// good
return Collections.emptyList();
// bad
return List.of("foo");
// good
return Collections.singletonList("foo");
It is recommended to declare variable as final- not strict but rather project convention to keep the code safe.
// bad
String value = "value";
// good
final String value = "value";
Results of long ternary operators should be on separate lines:
// bad
boolean result = someVeryVeryLongConditionThatForcesLineWrap ? firstResult
: secondResult;
// good
boolean result = someVeryVeryLongConditionThatForcesLineWrap
? firstResult
: secondResult;
Not so strict, but short ternary operations should be on one line:
// bad
boolean result = someShortCondition
? firstResult
: secondResult;
// good
boolean result = someShortCondition ? firstResult : secondResult;
Do not rely on operator precedence in boolean logic, use parenthesis instead. This will make code simpler and less error-prone.
// bad
final boolean result = a && b || c;
// good
final boolean result = (a && b) || c;
Try to avoid hard-readable multiple nested method calls:
// bad
int resolvedValue = resolveValue(fetchExternalJson(url, httpClient), populateAdditionalKeys(mainKeys, keyResolver));
// good
String externalJson = fetchExternalJson(url, httpClient);
List<Key> additionalKeys = fetchAdditionalKeys(mainKeys, keyResolver);
int resolvedValue = resolveValue(externalJson, additionalKeys);
Try not to retrieve same data more than once:
// bad
if (getData() != null) {
final Data resolvedData = resolveData(getData());
...
}
// good
final Data data = getData();
if (data != null) {
final Data resolvedData = resolveData(data);
...
}
If you're dealing with incoming data, please be sure to check if the nested object is not null before chaining.
// bad
final ExtRequestTargeting targeting = bidRequest.getExt().getPrebid().getTargeting();
// good
final ExtRequest requestExt = bidRequest.getExt();
final ExtRequestPrebid prebid = requestExt != null ? requestExt.getPrebid() : null;
final ExtRequestTargeting targeting = prebid != null ? prebid.getTargeting() : null;
For convenience, the org.prebid.server.util.ObjectUtil helper can be used for such kind of operations.
We are trying to get rid of long chains of null checks, which are described in suggestion above, in favor of Java Optional usage.
Don't leave commented code (don't think about the future).
// bad
// String iWillUseThisLater = "never";
You can always add it later when it will be really desired.
It is strictly prohibited to log any kind of private data about publisher, exchanges or similar sensitive information. The idea is to keep this open-source project safe as far as possible.
Try to write new bidders in the same manner with existing adapters. Utilize sample bidder code or use GenericBidder as a reference.
This is needed because bidder adapters tend to be modified frequently. In world where each bidder is written using different coding styles and techniques, maintainers would need to spend long time to understand bidders code before adding any modifications.
On the other hand, if each bidder adapter is written using common constructs, it is easy to review and modify bidders fast.
The code should be covered over 90%.
The common way for writing tests has to comply with given-when-then style.
// given
final BidRequest bidRequest = BidRequest.builder().id("").build();
// when
final ValidationResult result = requestValidator.validate(bidRequest);
// then
assertThat(result.getErrors()).containsOnly("request missing required field: \"id\"");
where:
given- initial state, data or conditions.when- stimulus: some action against the system under test.then- expectations/assertions.
The team decided to use name target for class instance under test.
Unit tests should be as granular as possible. Try to split unit tests into smaller ones until this is impossible to do.
// bad
@Test
public void testFooBar() {
// when
final String foo = service.getFoo();
final String bar = service.getBar();
// then
assertThat(foo).isEqualTo("foo");
assertThat(bar).isEqualTo("bar");
}
// good
@Test
public void testFoo() {
// when
final String foo = service.getFoo();
// then
assertThat(foo).isEqualTo("foo");
}
@Test
public void testBar() {
// when
final String bar = service.getBar();
// then
assertThat(bar).isEqualTo("bar");
}
This also applies to cases where same method is tested with different arguments inside single unit test. Note: This represents the replacement we have selected for parameterized testing.
// bad
@Test
public void testFooFirstSecond() {
// when
final String foo1 = service.getFoo(1);
final String foo2 = service.getFoo(2);
// then
assertThat(foo1).isEqualTo("foo1");
assertThat(foo2).isEqualTo("foo2");
}
// good
@Test
public void testFooFirst() {
// when
final String foo1 = service.getFoo(1);
// then
assertThat(foo1).isEqualTo("foo1");
}
@Test
public void testFooSecond() {
// when
final String foo2 = service.getFoo(2);
// then
assertThat(foo2).isEqualTo("foo2");
}
Name unit tests meaningfully. Test names should give brief description of what unit test tries to check.
It is also recommended to structure test method names with this scheme:
name of method that is being tested, word should, what a method should return.
If a method should return something based on a certain condition, add word when and description of a condition.
// bad
@Test
public void doSomethingTest() {
// when and then
assertThat(service.processData("data")).isEqualTo("result");
}
// good
@Test
public void processDataShouldReturnResultWhenInputIsData() {
// when and then
assertThat(service.processData("data")).isEqualTo("result");
}
Place data used in test as close as possible to test code. This will make tests easier to read, review and understand.
// bad
@Test
public void testFoo() {
// given
final String fooData = getSpecificFooData();
// when and then
assertThat(service.processFoo(fooData)).isEqualTo(getSpecificFooResult());
}
// good
@Test
public void testFoo() {
// given
final String fooData = "fooData";
// when and then
assertThat(service.processFoo(fooData)).isEqualTo("fooResult");
}
This point also implies the next one.
Since we are trying to improve test simplicity and readability and place test data close to tests, we decided to avoid usage of top level constants where it is possible. Instead, just inline constant values.
// bad
public class TestClass {
private static final String CONSTANT_1 = "foo";
...
private static final String CONSTANT_N = "bar";
// A bunch of other tests
@Test
public void testFoo() {
// when and then
assertThat(service.foo(CONSTANT_1)).isEqualTo(CONSTANT_N);
}
}
// good
public class TestClass {
// A bunch of other tests
@Test
public void testFoo() {
// when and then
assertThat(service.foo("foo")).isEqualTo("bar");
}
}
Don't use real information in tests, like existing endpoint URLs, account IDs, etc.
// bad
String ENDPOINT_URL = "https://prebid.org";
// good
String ENDPOINT_URL = "https://test-endpoint.url";
Along with regular unit-tests bidder's writer should provide smoke (historically we call them integration) tests.
Those tests are located at src/test/java/org/prebid/server/it folder.
The idea behind the smoke bidder test is to verify PBS can start up with supplied bidder configuration and to check the
simplest basic happy-path scenario which bidder code should do. Thus, the OpenRTB JSON request file (see the examples
in src/test/resources/org/prebid/server/it/openrtb2 folder)might contain exactly single bidder under testing and one
impression with single media type.
{
"id": "request_id",
"imp": [
{
"id": "imp_id",
"banner": {
"w": 320,
"h": 250
},
"ext": {
"bidder_name": {
"param1": "value1"
}
}
}
],
"tmax": 5000,
"regs": {
"ext": {
"gdpr": 0
}
}
}All possible scenarios for testing functionality must be covered by bidder's unit-tests.