Hi, community. IMO, 'null-process-strategy' in the project should be fitting: - Make codes clear and readable, not more puzzle - Satisfy common design pattern, such as hiddening all certain implement details in factory method
So, there are my suggestions: 1. Cases 1.1/1.2/1.4, the null-check need be encapsulated to a suitable override method. 2. Case 2.1, Objects.isNull in jdk8 usually used in lambda express as a method refer, only a independent null-check is ok(caes 2.2 can be replace with it). Others(defaultValues, Exceptions) are fine for me. At 2020-03-29 18:25:22, "Myth" <[email protected]> wrote: >Hi,All the friends of ShardingShpere community. >We judge the use of null in ShardingShpere, sorted out 7 types. >We have suggested changes to some of the code, and we hope that the community >will provide your valuable Suggestions as well. > >1 : To determine whether the object is null, then need to transform it. Here >are some examples. > > 1.1 > current code ：result.setUrl(null == dataSourceName ? > databaseEnvironment.getURL() : databaseEnvironment.getURL(dataSourceName)) > advice code > ：Optional.ofNullable(dataSourceName).map(databaseEnvironment::getURL).orElse(databaseEnvironment.getURL()) > > 1.2 > current code ： return null == loadBalanceStrategyConfiguration > ? serviceLoader.newService() > : > serviceLoader.newService(loadBalanceStrategyConfiguration.getType(), > loadBalanceStrategyConfiguration.getProperties()); > advice code ： return Optional.ofNullable(loadBalanceStrategyConfiguration) > .map(e -> > serviceLoader.newService(e.getType(),e.getProperties())) > .orElse(serviceLoader.newService()) > > 1.3 > current code ： return null == > shardingRuleConfig.getDefaultKeyGeneratorConfig() > ? null : > shardingRuleConfig.getDefaultKeyGeneratorConfig().getColumn(); > advice code ： return > Optional.ofNullable(shardingRuleConfig.getDefaultKeyGeneratorConfig()) > > .map(KeyGeneratorConfiguration::getColumn).orElse(null) > > 1.4 > current code ： return null == shardingStrategyConfiguration > ? new NoneShardingStrategy() : > ShardingStrategyFactory.newInstance(shardingStrategyConfiguration); > advice code ： return Optional.ofNullable(shardingStrategyConfiguration) > > .map(ShardingStrategyFactory::newInstance).orElse(new NoneShardingStrategy()) > > >2 : Object is judged with null. Here are some examples. > > 2.1 > current code ： > > public void xxx（Object o）{ > if(null == o){ > retrun; > } > } > > 2.2 > current code ： > > public boolean wasNull() { > return null == currentRow; > } > > advice : Use the objects.isNull() provided by JDK8. > > >3 : Use the ternary operator to determine whether the object is null, then >returns itself or other. Here are some examples. > > 3.1 > current code ： this.loadBalanceAlgorithm = null == loadBalanceAlgorithm ? > new MasterSlaveLoadBalanceAlgorithmServiceLoader().newService() : > loadBalanceAlgorithm; > advice code ： this.loadBalanceAlgorithm = > Optional.ofNullable(loadBalanceAlgorithm).orElse(new > MasterSlaveLoadBalanceAlgorithmServiceLoader().newService()) > > 3.2 > current code ： currentDataSourceName = null == currentDataSourceName ? > shardingRule.getShardingDataSourceNames().getRandomDataSourceName() : > currentDataSourceName; > advice code ： currentDataSourceName = > Optional.ofNullable(currentDataSourceName).orElse(shardingRule.getShardingDataSourceNames().getRandomDataSourceName()) > > 3.3 > current code ： return null == tableRule.getDatabaseShardingStrategy() ? > defaultDatabaseShardingStrategy : tableRule.getDatabaseShardingStrategy(); > advice code ： return > Optional.ofNullable(tableRule.getDatabaseShardingStrategy()).orElse(defaultDatabaseShardingStrategy) > > 3.4 > current code ： return null == results.get(0) ? 0 : results.get(0); > advice code ： return Optional.ofNullable(results.get(0)).orElse(0); > > 3.5 > current code ： return null == getSqlStatement().getTable() ? > Collections.emptyList() : > Collections.singletonList(getSqlStatement().getTable()); > advice code ： return > Optional.ofNullable(getSqlStatement().getTable()).map(Collections::singletonList).orElse(Collections.emptyList()); > > >4 : Get the value of the Map by key, and then determines that the value is >null. Here are some examples. > > 4.1 > current code ： > > public Collection<String> getActualTableNames(final String > targetDataSource) { > Collection<String> result = > datasourceToTablesMap.get(targetDataSource); > if (null == result) { > result = Collections.emptySet(); > } > return result; > } > > advice code ： Use Map.getOrDefault(). > > public Collection<String> getActualTableNames(final String > targetDataSource) { > return datasourceToTablesMap.getOrDefault(targetDataSource, > Collections.emptySet()); > } > > >5 : To determine whether the object is null, but returns Optional wrapped. >Here are some examples. > > 5.1 > current code ： return null == alias ? Optional.empty() : > Optional.ofNullable(alias.getIdentifier().getValue()); > advice code ： return Optional.ofNullable(alias).map(e -> > e.getIdentifier().getValue()); > > >6 : Determine if the object is null, throw exception, and proceed to the next >step. Here are some examples. > > 6.1 > current code ： > > private Collection<String> doSharding(final Collection<String> > availableTargetNames, final RangeRouteValue<?> shardingValue) { > if (null == rangeShardingAlgorithm) { > throw new UnsupportedOperationException("Cannot find range > sharding strategy in sharding rule."); > } > return rangeShardingAlgorithm.doSharding(availableTargetNames, > new RangeShardingValue(shardingValue.getTableName(), > shardingValue.getColumnName(), shardingValue.getValueRange())); > } > > advice code ： > > private Collection<String> doSharding(final Collection<String> > availableTargetNames, final RangeRouteValue<?> shardingValue) { > return Optional.ofNullable(rangeShardingAlgorithm).map(e -> > e.doSharding(availableTargetNames, > new RangeShardingValue(shardingValue.getTableName(), > shardingValue.getColumnName(), shardingValue.getValueRange()))) > .orElseThrow(()-> new > UnsupportedOperationException("Cannot find range sharding strategy in > sharding rule.")); > } > >7 : Judge Collection is Empty. Here are some examples. > > 7.1 > current code ： > > private boolean isEmptyDataNodes(final List<String> dataNodes) { > return null == dataNodes || dataNodes.isEmpty(); > } > > advice ：Unify judgment with Collection Tool. > >Thank you for wasting your precious time reading, and the ShardingShpere >community welcomes your best Suggestions for the above code.
