(Understanding OpenSearch/ElasticSearch is not important for this question, but the below information provides some context).
I am using the new opensearch-java (version 2.4.0) library to interact with the OpenSearch cluster. I am trying to build the TypeMapping
using Builder pattern, so that I can pass these properties mapping when creating a new index in OpenSearch like so:
// client will call this method when creating index.
public void createIndex(String name, TypeMapping typeMappings) {
CreateIndexRequest createIndexRequest = new CreateIndexRequest.Builder()
.index(name)
.mappings(typeMappings)
.build();
createIndex(openSearchClient, createIndexRequest);
}
private static void createIndex(OpenSearchClient osClient, CreateIndexRequest createIndexRequest) {
osClient.indices().create(createIndexRequest);
}
I have the following code to use the Builder pattern to build the TypeMapping
.
package test;
import java.util.Map;
import org.opensearch.client.opensearch._types.mapping.Property;
import org.opensearch.client.opensearch._types.mapping.TypeMapping;
import org.opensearch.client.opensearch._types.mapping.DynamicMapping;
public class TypeMappingBuilder {
private DynamicMapping dynamic;
private final Map<String, Property> properties;
public TypeMappingBuilder() {
this.properties = new HashMap<>();
}
public TypeMappingBuilder disableDynamic() {
this.mapping = DynamicMapping.Strict;
return this;
}
public TypeMappingBuilder putTypeProperty(
String name,
Property.Kind type,
boolean shouldIndex,
Map<String, Property> subTypeProperties) {
this.properties.put(name, getTypeProperty(type, shouldIndex, subTypeProperties));
return this;
}
private Property getTypeProperty(
Property.Kind type,
boolean shouldIndex,
Map<String, Property> subTypeProperties) {
if (type == Property.Kind.Text) {
// Here, in (p -> p), p is the TextProperty.Builder() provided by OpenSearch Java client
return new Property.Builder().text(p -> p.index(shouldIndex).fields(subTypeProperties)).build();
}
if (type == Property.Kind.Keyword) {
// Here, in (p -> p), p is the KeywordProperty.Builder() provided by OpenSearch Java client
return new Property.Builder().keyword(p -> p.index(shouldIndex).fields(subTypeProperties)).build();
}
//...
//...
//... and so forth, there are more than 20 of the Property.Kind values where I can use the if condition
}
public TypeMapping build() {
return new TypeMapping.Builder()
.dynamic(dynamic)
.properties(properties)
.build();
}
}
The issue is that too many if statement here (in method getTypeProperty
) will cause code smell and also if a new type is added in the future, then I have to update getTypeProperty
method to apply for the new type. Also, in this new Property.Builder().{specificType}({specificTypeBuilder}).build()
, I cannot use polymorphism as there is different method names for each {specificType}
and different type Builder for each {specificTypeBuilder}
. I don't see any other options than to use if statements to create different Property based on a specific type. I thought of using static Map like so and access specific Property from the Map, but would only work if I didn't have subTypeProperties
to put in for sub fields
(but, it will have more memory and type overhead):
Map<PropertyHolder, Property> propertyMap = ImmutableMap.<PropertyHolder, Property>builder()
.put(new PropertyHolder(Property.Kind.Text, true), new Property.Builder()
.text(p -> p.index(true)).build())
.put(new PropertyHolder(Property.Kind.Text, false), new Property.Builder()
.text(p -> p.index(false)).build())
.put(new PropertyHolder(Property.Kind.Keyword, true), new Property.Builder()
.keyword(p -> p.index(true)).build())
.put(new PropertyHolder(Property.Kind.Keyword, false), new Property.Builder()
.keyword(p -> p.index(false)).build())
...
...
...
.build();
final class PropertyHolder {
private final Property.Kind type;
private final boolean index;
private PropertyHolder(Property.Kind type, boolean index) {
this.type = type;
this.index = index;
}
public static Property getTypeProperty(Property.Kind type, boolean shouldIndex) {
return propertyMap.get(new PropertyHolder(type, shouldIndex));
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
PropertyHolder that = (PropertyHolder) o;
return type == that.type && index == that.index;
}
@Override
public int hashCode() {
return Objects.hash(type, index);
}
}
Another option that can actually work was to create my own enum and have a method defined for each enum value that returns a specific Property based on the enum value (Strategy pattern). But, that would be an abuse of enums and probably code smell. Here is what I am talking about:
public enum Type {
TEXT {
@Override
public Property getTypeProperty(boolean shouldIndex, Map<String, Property> subTypeProperties) {
return new Property.Builder().text(p -> p.index(shouldIndex).fields(subTypeProperties)).build();
}
},
KEYWORD {
@Override
public Property getTypeProperty(boolean shouldIndex, Map<String, Property> subTypeProperties) {
return new Property.Builder().keyword(p -> p.index(shouldIndex).fields(subTypeProperties)).build();
}
},
...,
...,
...;
public abstract Property getTypeProperty(boolean shouldIndex, Map<String, Property> subTypeProperties);
}
What design pattern can I use in this scenario to avoid code smell? Thank you!
EDIT:
In the scenario that I am working on, the client can either create TypeMapping
using TypeMappingBuilder
and then, pass that to the createIndex
request, like so (added based on the answer mentioned by BionicCode):
TypeMapping typeMappings = new TypeMappingBuilder();
builder.putTextProperty(...)
.putTextProperty(...)
.putKeywordProperty(...)
.build();
createIndex(indexName, typeMappings);
or using MappingFactory
like so:
private static final Map<String, Property> MAPPINGS = ImmutableMap.<String, Property>builder()
.putAll(MappingFactory.getMapping(Memory.class))
.putAll(MappingFactory.getMapping(Cpu.class))
...
...
...
.build();
TypeMapping typeMappings = new TypeMapping.Builder().properties(MAPPINGS).build();
createIndex(indexName, typeMappings);
Here, the Memory.class
or Cpu.class
POJO looks like this:
class Memory {
@TypeAnnotation(name = "mem_type", type = Type.KEYWORD, index = FALSE)
private final String memType;
@TypeAnnotation(name = "bytes", type = Type.LONG, index = true)
private final long bytes;
...
...
...
}
And, the TypeAnnotation
in my library looks like this:
@interface TypeAnnotation {
String name();
Type type();
boolean index();
public static final Map<Type, Factory> TYPE_FACTORY_MAP = ImmutableMap.<Type, Factory>builder()
.put(Type.TEXT, new TextFactory())
.put(Type.KEYWORD, new KeywordFactory())
...
...
...
.build();
}
Now, in the scenario where the Client uses TypeMappingBuilder
, I can setup putKeywordProperty
, putTextProperty
, etc. methods for the client to use to add specific property to the properties Map
before building TypeMapping
.
But, when the Client calls MappingFactory.getMapping(Memory.class)
, the current implementation for getMapping(Class)
method is like so:
class MappingFactory {
public static Map<String, Property> getMapping(Class<?> clazz) {
Walker walker = new Walker();
// this will walk the fields and add each Property to Walker properties Map.
ReflectionUtils.doWithFields(clazz, walker);
return walker.getMapping();
}
// FieldCallback is provided by Spring framework as part of it's ReflectionUtils
private static final class Walker implements FieldCallback {
private final ImmutableMap.Builder<String, Property> properties = ImmutableMap.builder();
public ImmutableMap<String, Property> getMapping() {
return this.properties.build();
}
@Override
public void doWith(java.lang.reflect.Field field) {
TypeAnnotation typeAnnotation = (TypeAnnotation) field.getAnnotation(TypeAnnotation.class);
String name = typeAnnotation.name();
Type type = typeAnnotation.type();
boolean index = typeAnnotation.index();
// map defined in TypeAnnotation that contains <Type, Factory> and will return a specific property Factory (implements Factory) that you can use to create Property.
Factory factory = TYPE_FACTORY_MAP.get(type);
Property property = factory.getProperty(index);
this.properties.put(name, property);
}
}
}
interface Factory {
Property getProperty(boolean index);
Property getProperty(boolean index, Map<String, Property> subProperties);
}
class KeywordFactory implements Factory {
@Override
public Property getProperty(boolean index) {
return new Property.Builder().keyword(p -> p.index(index)).build();
}
@Override
public Property getProperty(boolean index, Map<String, Property> subProperties) {
return new Property.Builder().keyword(p -> p.index(index).fields(subProperties)).build();
}
}
The issue is that I have to use either if/else or switch or factory pattern (I am using factory pattern here in doWith
method and storing each factory in a Map<Type, Factory>
that is defined in TypeAnnotation
). Is there any other solution or a solution that is better when the client have the options to either use TypeMappingBuilder
or MappingFactory
? Thanks!
Usually, the caller of your API knows all the details of the context. You only have to expose a complete path for each supported context.
As a result the client code now explicitly selects the method/execution path instead of selecting an enum value.
This will make your API bigger opposed to a more generic API. But if it is about code paths, generic APIs are not useful (see your problem).
generic APIs only help in terms of type compatibility (input/output).
In terms of execution paths, you always need the client of your API to select an execution path: either in form of a parameter (you current choice) or by explicitly invoking the desired functionality.
The latter provides:
Property.Kind
enum (additional type only introduced to select the execution path).The solution is to allow the builder to explicitly fork the execution path. This eliminates the Property.Kind
enum (previously used to identify the selected execution path) and the getTypeProperty
method (previously used to execute the specialized execution path). Now the client of the TypeMappingBuilder
API is responsible to select the execution path explicitly by calling the related method instead of passing the enum identifier.
The result is a more verbose client code (in terms of enhanced readability) and a more convenient API.
It's the only solution that improves the API, for example in contrast to alternative solutions like looking up instances in a hash table.
A hash table solution only works for object lookup. A hash table still won't solve more complex scenarios. In addition a hash table solution adds more memory and types overhead - while we usually can live with the memory overhead, the additional types we have to implement add unnecessary complexity).
So instead of adding more complexity to your internal code, simply clean up the API:
Usage example:
// Easier to read client code, where the intend is communicated via method names instead of argument names.
// Even the argument name is meaning less
// if the reader does not know that the argument is an actual execution path selector.
// To know this, he would have to read the API documentation.
// The method name allows the reader to instantly know the purpose
// and the author which method to invoke.
TypeMappingBuilder builder = new TypeMappingBuilder();
builder.putTextProperty(...)
.putTextProperty(...)
.putKeywordProperty(...);
TypeMappingBuilder.java
public class TypeMappingBuilder {
private DynamicMapping dynamic;
private final Map<String, Property> properties;
public TypeMappingBuilder() {
this.properties = new HashMap<>();
}
public TypeMappingBuilder disableDynamic() {
this.mapping = DynamicMapping.Strict;
return this;
}
public TypeMappingBuilder putTextProperty(
String name,
boolean shouldIndex,
Map<String, Property> subTypeProperties) {
Property textProperty = new Property.Builder().text(p -> p.index(shouldIndex).fields(subTypeProperties)).build();
this.properties.put(name, textProperty);
return this;
}
public TypeMappingBuilder putKeywordProperty(
String name,
boolean shouldIndex,
Map<String, Property> subTypeProperties) {
Property keywordProperty = new Property.Builder().keyword(p -> p.index(shouldIndex).fields(subTypeProperties)).build();
this.properties.put(name, keywordProperty);
return this;
}
public TypeMapping build() {
return new TypeMapping.Builder()
.dynamic(dynamic)
.properties(properties)
.build();
}
}