Skip to content

code-style.md

Latest commit

History

History
656 lines (480 loc) · 14.6 KB

code-style.md

File metadata and controls

656 lines (480 loc) · 14.6 KB
Raw

Code Style

The intent here is to maintain a common style across the project and rely on the process to enforce it instead of individuals.

Automated code style check

The pom.xml is configured to enforce a coding style defined in checkstyle.xml when maven validate phase executed.

Formatting

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.

Code conventions

Max line length

Line length is limited to 120 columns for *.java files. Other file types are unrestricted.

Transitive dependencies

Don't use transitive dependencies in project code. If it needed, recommended adding a library as a dependency of Maven in pom.xml directly.

Add library as maven dependency

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>

Avoid wildcards in imports

Do not use wildcard in imports because they hide what exactly is required by the class.

// badimportjava.util.*;
// goodimportjava.util.HashMap;
importjava.util.Map;

Variable and method naming

Prefer to use camelCase naming convention for variables and methods.

// badStringaccount_id = "id";
// goodStringaccountId = "id";

Name of variable should be self-explanatory:

// badStrings = resolveParamA();
// goodStringresolvedParamA = 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:

// badMap<Imp, ExtImp> map = getData();
// goodMap<Imp, ExtImp> impToExt = getData();

Immutability in DTO classes

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.

// badpublicclassMyDto {
privatefinalStringvalue;
publicMyDto(Stringvalue) {
this.value = value;
}
publicvoidsetValue(Stringvalue) {
this.value = value;
}
publicStringgetValue() {
returnvalue;
}
}
// and later usagefinalMyDtomyDto = newMyDto("value");
myDto.setValue("newValue");
// good@Builder(toBuilder=true)
@Value(staticConstructor="of")
publicclassMyDto {
Stringvalue;
}
// and later usagefinalMyDtomyDto = MyDto.of("value");
finalMyDtoupdatedDto = myDto.toBuilder().value("newValue").build();

Variables types

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.

// badfinalvarresult = getResult();
// goodfinalDataresult = getResult();

Parenthesis placement

Enclosing parenthesis should be placed on expression end.

// badmethodCall(
longlistofarguments
);
// goodmethodCall(
longlistofarguments);

This also applies for nested expressions.

// bad methodCall(
nestedCall(
longlistofarguments
)
);
// goodmethodCall(
nestedCall(
longlistofarguments));

Method placement

Please, place methods inside a class in call order.

// badpublicinterfaceTest {
voida();
voidb();
}
publicclassTestImplimplementsTest {
@Overridepublicvoida() {
c();
}
@Overridepublicvoidb() {
d();
}
privatevoidd() {
...
}
privatevoidc() {
...
}
}
// good publicinterfaceTest {
voida();
voidb();
}
publicclassTestImplimplementsTest {
@Overridepublicvoida() {
c();
}
privatevoidc() {
...
}
@Overridepublicvoidb() {
d();
}
privatevoidd() {
...
}
}
Explanationofanexample: Defineinterfacefirstmethod, thenallmethodsthatitiscalling, thensecondmethodofaninterfaceandallmethodsthatitiscalling, andsoon.

Separation of method signature definition and body

Not strict, but methods with long parameters list, that cannot be placed on single line, should add empty line before body definition.

// badpublicstaticvoidmethod(
parametersdefinitions) {
startofbodydefinition// goodpublicstaticvoidmethod(
parametersdefinitions) {
startofbodydefinition

Use special methods for short collections initializations

Use collection literals where it is possible to define and initialize collections.

// bad finalList<String> foo = newArrayList();
foo.add("foo");
foo.add("bar");
// good finalList<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 returnList.of();
// goodreturnCollections.emptyList();
// badreturnList.of("foo");
// goodreturnCollections.singletonList("foo");

Make variables final

It is recommended to declare variable as final- not strict but rather project convention to keep the code safe.

// badStringvalue = "value";
// goodfinalStringvalue = "value";

Ternary expressions

Results of long ternary operators should be on separate lines:

// badbooleanresult = someVeryVeryLongConditionThatForcesLineWrap ? firstResult
: secondResult;
// goodbooleanresult = someVeryVeryLongConditionThatForcesLineWrap
? firstResult
: secondResult;

Not so strict, but short ternary operations should be on one line:

// badbooleanresult = someShortCondition
? firstResult
: secondResult;
// goodbooleanresult = someShortCondition ? firstResult : secondResult;

Complex boolean logic

Do not rely on operator precedence in boolean logic, use parenthesis instead. This will make code simpler and less error-prone.

// badfinalbooleanresult = a && b || c;
// goodfinalbooleanresult = (a && b) || c;

Nested method calls

Try to avoid hard-readable multiple nested method calls:

// badintresolvedValue = resolveValue(fetchExternalJson(url, httpClient), populateAdditionalKeys(mainKeys, keyResolver));
// goodStringexternalJson = fetchExternalJson(url, httpClient);
List<Key> additionalKeys = fetchAdditionalKeys(mainKeys, keyResolver);
intresolvedValue = resolveValue(externalJson, additionalKeys);

Data retrieval calls of same result

Try not to retrieve same data more than once:

// badif (getData() != null) {
finalDataresolvedData = resolveData(getData());
...
}
// goodfinalDatadata = getData();
if (data != null) {
finalDataresolvedData = resolveData(data);
...
}

Check for NULL

If you're dealing with incoming data, please be sure to check if the nested object is not null before chaining.

// badfinalExtRequestTargetingtargeting = bidRequest.getExt().getPrebid().getTargeting();
// goodfinalExtRequestrequestExt = bidRequest.getExt();
finalExtRequestPrebidprebid = requestExt != null ? requestExt.getPrebid() : null;
finalExtRequestTargetingtargeting = prebid != null ? prebid.getTargeting() : null;

For convenience, the org.prebid.server.util.ObjectUtil helper can be used for such kind of operations.

Optional usages

We are trying to get rid of long chains of null checks, which are described in suggestion above, in favor of Java Optional usage.

Garbage code

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.

Privacy

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.

Bidder implementation

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.

Tests

The code should be covered over 90%.

Given-When-Then approach

The common way for writing tests has to comply with given-when-then style.

// givenfinalBidRequestbidRequest = BidRequest.builder().id("").build();
// whenfinalValidationResultresult = requestValidator.validate(bidRequest);
// thenassertThat(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.

Testing instance naming

The team decided to use name target for class instance under test.

Tests granularity

Unit tests should be as granular as possible. Try to split unit tests into smaller ones until this is impossible to do.

// bad@TestpublicvoidtestFooBar() {
// whenfinalStringfoo = service.getFoo();
finalStringbar = service.getBar();
// thenassertThat(foo).isEqualTo("foo");
assertThat(bar).isEqualTo("bar");
}
// good@TestpublicvoidtestFoo() {
// whenfinalStringfoo = service.getFoo();
// thenassertThat(foo).isEqualTo("foo");
}
@TestpublicvoidtestBar() {
// whenfinalStringbar = service.getBar();
// thenassertThat(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@TestpublicvoidtestFooFirstSecond() {
// whenfinalStringfoo1 = service.getFoo(1);
finalStringfoo2 = service.getFoo(2);
// thenassertThat(foo1).isEqualTo("foo1");
assertThat(foo2).isEqualTo("foo2");
}
// good@TestpublicvoidtestFooFirst() {
// whenfinalStringfoo1 = service.getFoo(1);
// thenassertThat(foo1).isEqualTo("foo1");
}
@TestpublicvoidtestFooSecond() {
// whenfinalStringfoo2 = service.getFoo(2);
// thenassertThat(foo2).isEqualTo("foo2");
}

Unit tests naming

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@TestpublicvoiddoSomethingTest() {
// when and thenassertThat(service.processData("data")).isEqualTo("result");
}
// good@TestpublicvoidprocessDataShouldReturnResultWhenInputIsData() {
// when and thenassertThat(service.processData("data")).isEqualTo("result");
}

Place test data as close as possible to test

Place data used in test as close as possible to test code. This will make tests easier to read, review and understand.

// bad@TestpublicvoidtestFoo() {
// givenfinalStringfooData = getSpecificFooData();
// when and thenassertThat(service.processFoo(fooData)).isEqualTo(getSpecificFooResult());
}
// good@TestpublicvoidtestFoo() {
// givenfinalStringfooData = "fooData";
// when and thenassertThat(service.processFoo(fooData)).isEqualTo("fooResult");
}

This point also implies the next one.

Avoid class level constants in test classes

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.

// badpublicclassTestClass {
privatestaticfinalStringCONSTANT_1 = "foo";
...
privatestaticfinalStringCONSTANT_N = "bar";
// A bunch of other tests@TestpublicvoidtestFoo() {
// when and thenassertThat(service.foo(CONSTANT_1)).isEqualTo(CONSTANT_N);
}
}
// goodpublicclassTestClass {
// A bunch of other tests@TestpublicvoidtestFoo() {
// when and thenassertThat(service.foo("foo")).isEqualTo("bar");
}
}

Real data in tests

Don't use real information in tests, like existing endpoint URLs, account IDs, etc.

// badStringENDPOINT_URL = "https://prebid.org";
// goodStringENDPOINT_URL = "https://test-endpoint.url";

Bidder smoke tests

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.