Cpt_S_581 Erik Jepsen's Refactoring - WSU-CptS-581-2025/zerocode GitHub Wiki
- ScenarioExecutionState.java - 4
- StepExecutionState.java - 3
- ZeroCodeAssertionsProcessorImpl.java - 40
- ZeroCodeExternalFileProcessorImpl.java - 16
- ZeroCodeParameterizedProcessorImpl.java - 13
- Class % - 87%
- Method % - 85%
- Line % - 78%
- Branch % 72%
- ScenarioExecutionState.java - 4
- StepExecutionState.java - 3
- ZeroCodeAssertionsProcessorImpl.java - 17
- ZeroCodeExternalFileProcessorImpl.java - 16
- ZeroCodeParameterizedProcessorImpl.java - 12
- Class % - 87%
- Method % - 92%
- Line % - 85%
- Branch % 78%
I was looking at this method
public Optional<StepExecutionState> getExecutedStepState(String stepName) {
return Optional.of(allStepsLinkedMap.get(stepName));
}and it occurred to me to check to see what Map.get() does if it can't find the key. It returns null. Now the map itself will never cause a null pointer exception because it's instantiated during the class's initialization, but .get() could still return a null value. I sliced thru the code to see where and how it was being used. I found one use...
Optional<StepExecutionState> retryWithStepExecState = scenarioExecutionState.getExecutedStepState(stepName);Unfortunately, if get() returned null, the line in ScenarioExecutionState.java I'd been looking at would produce a null pointer exception, so I altered the code to...
public Optional<StepExecutionState> getExecutedStepState(String stepName) {
return Optional.ofNullable(allStepsLinkedMap.get(stepName));
}I found another instance where the same code could be replaced with the same static method as before
return (new StringSubstitutor(paramMap)).replace(scenarioStateTemplate);became...
return SmartUtils.resolveToken(scenarioStateTemplate, paramMap);There is a stream method that does what this method does in one line.
private boolean hasNoTypeCast(String resolvedJson) {
long foundCount = fieldTypes.stream().filter(resolvedJson::contains).count();
return foundCount <= 0;
}Refactored to...
private boolean hasNoTypeCast(String resolvedJson) {
return fieldTypes.stream().noneMatch(resolvedJson::contains);
}I removed a bit of unnecessary regex code from a string. The opening square brace needs to be escaped, but not its mate, unless there is a more complex regex being made.
final String LEAF_VAL_REGEX = "\\$[.](.*)\\$VALUE\\[\\d\\]";became...
final String LEAF_VAL_REGEX = "\\$[.](.*)\\$VALUE\\[\\d]";Duplicate code was found at the end of this method. A static method already existed to do what this method spent the last two lines doing.
StringSubstitutor sub = new StringSubstitutor(paramMap);
return sub.replace(jsonString);became...
return SmartUtils.resolveToken(jsonString, paramMap);This method had a long and nested If/Else statement buried inside a repeating lambda. I decided to break this up first by pulling out the lion's share of the if statements that had something in common: if (value instanceof String. I put those into their own method to start. I left in place the failsafe of creating a FieldIsValueAsserter instance, and noticed that this was still looking large and unwieldy, so I broke them up further by whether the asserter they would return was a string-type, number-type or date-time-type. I also cut down on the duplicate code by creating a generic for loop, putting the variables into a Map and iterating over the map's Entries with the loop.
public List<JsonAsserter> createJsonAsserters(String resolvedAssertionJson) {
List<JsonAsserter> asserters = new ArrayList<>();
try {
JsonNode jsonNode = mapper.readTree(resolvedAssertionJson);
Map<String, Object> createFieldsKeyValuesMap = createAssertionKV(jsonNode, "$.");
for (Map.Entry<String, Object> entry : createFieldsKeyValuesMap.entrySet()) {
String path = entry.getKey();
Object value = entry.getValue();
JsonAsserter asserter;
if (ASSERT_VALUE_NOT_NULL.equals(value) || ASSERT_VALUE_IS_NOT_NULL.equals(value)) {
asserter = new FieldIsNotNullAsserter(path);
} else if (value instanceof String && ((String) value).startsWith(ASSERT_VALUE_CUSTOM_ASSERT)) {
String expected = ((String) value).substring(ASSERT_VALUE_CUSTOM_ASSERT.length());
asserter = new FieldMatchesCustomAsserter(path, expected);
} else if (ASSERT_VALUE_NULL.equals(value) || ASSERT_VALUE_IS_NULL.equals(value)) {
asserter = new FieldIsNullAsserter(path);
} else if (ASSERT_VALUE_EMPTY_ARRAY.equals(value)) {
asserter = new ArrayIsEmptyAsserterImpl(path);
} else if (path.endsWith(ASSERT_PATH_SIZE)) {
path = path.substring(0, path.length() - ASSERT_PATH_SIZE.length());
if (value instanceof Number) {
asserter = new ArraySizeAsserterImpl(path, (Integer) value);
} else if (value instanceof String) {
asserter = new ArraySizeAsserterImpl(path, (String) value);
} else {
throw new RuntimeException(format("Oops! Unsupported value for .SIZE: %s", value));
}
} else if (value instanceof String) && ((String) value).startsWith(ASSERT_VALUE_CONTAINS_STRING)) {
String expected = ((String) value).substring(ASSERT_VALUE_CONTAINS_STRING.length());
asserter = new FieldContainsStringAsserter(path, expected);
} else if (value instanceof String && ((String) value).startsWith(ASSERT_VALUE_MATCHES_STRING)) {
String expected = ((String) value).substring(ASSERT_VALUE_MATCHES_STRING.length());
asserter = new FieldMatchesRegexPatternAsserter(path, expected);
} else if (value instanceof String && ((String) value).startsWith(ASSERT_VALUE_CONTAINS_STRING_IGNORE_CASE)) {
String expected = ((String) value).substring(ASSERT_VALUE_CONTAINS_STRING_IGNORE_CASE.length());
asserter = new FieldContainsStringIgnoreCaseAsserter(path, expected);
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_VALUE_EQUAL_TO_NUMBER)) {
String expected = ((String) value).substring(ASSERT_VALUE_EQUAL_TO_NUMBER.length());
asserter = new FieldHasEqualNumberValueAsserter(path, numberValueOf(expected));
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_VALUE_NOT_EQUAL_TO_NUMBER)) {
String expected = ((String) value).substring(ASSERT_VALUE_NOT_EQUAL_TO_NUMBER.length());
asserter = new FieldHasInEqualNumberValueAsserter(path, numberValueOf(expected));
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_VALUE_GREATER_THAN)) {
String expected = ((String) value).substring(ASSERT_VALUE_GREATER_THAN.length());
asserter = new FieldHasGreaterThanValueAsserter(path, numberValueOf(expected));
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_VALUE_LESSER_THAN)) {
String expected = ((String) value).substring(ASSERT_VALUE_LESSER_THAN.length());
asserter = new FieldHasLesserThanValueAsserter(path, numberValueOf(expected));
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_LOCAL_DATETIME_AFTER)) {
String expected = ((String) value).substring(ASSERT_LOCAL_DATETIME_AFTER.length());
asserter = new FieldHasDateAfterValueAsserter(path, parseLocalDateTime(expected));
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_LOCAL_DATETIME_BEFORE)) {
String expected = ((String) value).substring(ASSERT_LOCAL_DATETIME_BEFORE.length());
asserter = new FieldHasDateBeforeValueAsserter(path, parseLocalDateTime(expected));
} else if (value instanceof String && (value.toString()).startsWith(ASSERT_VALUE_ONE_OF) ||
value instanceof String && (value.toString()).startsWith(ASSERT_VALUE_IS_ONE_OF)) {
String expected = ((String) value).substring(ASSERT_VALUE_ONE_OF.length());
asserter = new FieldIsOneOfValueAsserter(path, expected);
} else {
asserter = new FieldHasExactValueAsserter(path, value);
}became...
public List<JsonAsserter> createJsonAsserters(String resolvedAssertionJson) {
List<JsonAsserter> asserters = new ArrayList<>();
try {
JsonNode jsonNode = mapper.readTree(resolvedAssertionJson);
Map<String, Object> createFieldsKeyValuesMap = createAssertionKV(jsonNode, "$.");
for (Map.Entry<String, Object> entry : createFieldsKeyValuesMap.entrySet()) {
String path = entry.getKey();
Object value = entry.getValue();
JsonAsserter asserter;
if (ASSERT_VALUE_NOT_NULL.equals(value) || ASSERT_VALUE_IS_NOT_NULL.equals(value)) {
asserter = new FieldIsNotNullAsserter(path);
} else if (ASSERT_VALUE_NULL.equals(value) || ASSERT_VALUE_IS_NULL.equals(value)) {
asserter = new FieldIsNullAsserter(path);
} else if (ASSERT_VALUE_EMPTY_ARRAY.equals(value)) {
asserter = new ArrayIsEmptyAsserterImpl(path);
} else if (path.endsWith(ASSERT_PATH_SIZE)) {
path = path.substring(0, path.length() - ASSERT_PATH_SIZE.length());
if (value instanceof Number) {
asserter = new ArraySizeAsserterImpl(path, (Integer) value);
} else if (value instanceof String) {
asserter = new ArraySizeAsserterImpl(path, (String) value);
} else {
throw new RuntimeException(format("Oops! Unsupported value for .SIZE: %s", value));
}
} else if (value instanceof String)
asserter = getStringAsserter(value, path);
else
asserter = new FieldHasExactValueAsserter(path, value);and...
protected JsonAsserter getStringAsserter(Object value, String path) {
String expected, valueString;
valueString = Objects.requireNonNull(value).toString();
Map<String, BiFunction<String, String, JsonAsserter>> tokenToStringMethodMap = new HashMap<>();
tokenToStringMethodMap.put(ASSERT_VALUE_CONTAINS_STRING, FieldContainsStringAsserter::new);
tokenToStringMethodMap.put(ASSERT_VALUE_MATCHES_STRING, FieldMatchesRegexPatternAsserter::new);
tokenToStringMethodMap.put(ASSERT_VALUE_CONTAINS_STRING_IGNORE_CASE, FieldContainsStringIgnoreCaseAsserter::new);
tokenToStringMethodMap.put(ASSERT_VALUE_IS_ONE_OF, FieldIsOneOfValueAsserter::new);
tokenToStringMethodMap.put(ASSERT_VALUE_ONE_OF, FieldIsOneOfValueAsserter::new);
tokenToStringMethodMap.put(ASSERT_VALUE_CUSTOM_ASSERT, FieldMatchesCustomAsserter::new);
for (Map.Entry<String, BiFunction<String, String, JsonAsserter>> entry : tokenToStringMethodMap.entrySet()) {
if (valueString.startsWith(entry.getKey())) {
expected = valueString.substring(entry.getKey().length());
return entry.getValue().apply(path, expected);
}
}
return getNumberAsserter(value, path);
}
protected JsonAsserter getNumberAsserter(Object value, String path) {
String expected, valueString;
valueString = value.toString();
Map<String, BiFunction<String, Number, JsonAsserter>> tokenToNumberMethodMap = new HashMap<>();
tokenToNumberMethodMap.put(ASSERT_VALUE_EQUAL_TO_NUMBER, FieldHasEqualNumberValueAsserter::new);
tokenToNumberMethodMap.put(ASSERT_VALUE_NOT_EQUAL_TO_NUMBER, FieldHasInEqualNumberValueAsserter::new);
tokenToNumberMethodMap.put(ASSERT_VALUE_GREATER_THAN, FieldHasGreaterThanValueAsserter::new);
tokenToNumberMethodMap.put(ASSERT_VALUE_LESSER_THAN, FieldHasLesserThanValueAsserter::new);
for (Map.Entry<String, BiFunction<String, Number, JsonAsserter>> entry : tokenToNumberMethodMap.entrySet()) {
if (valueString.startsWith(entry.getKey())) {
expected = valueString.substring(entry.getKey().length());
return entry.getValue().apply(path, numberValueOf(expected));
}
}
return getDateTimeAsserter(value, path);
}
protected JsonAsserter getDateTimeAsserter(Object value, String path) {
String expected, valueString;
valueString = value.toString();
Map<String, BiFunction<String, LocalDateTime, JsonAsserter>> tokenToDateTimeMethodMap = new HashMap<>();
tokenToDateTimeMethodMap.put(ASSERT_LOCAL_DATETIME_AFTER, FieldHasDateAfterValueAsserter::new);
tokenToDateTimeMethodMap.put(ASSERT_LOCAL_DATETIME_BEFORE, FieldHasDateBeforeValueAsserter::new);
for (Map.Entry<String, BiFunction<String, LocalDateTime, JsonAsserter>> entry : tokenToDateTimeMethodMap.entrySet()) {
if (valueString.startsWith(entry.getKey())) {
expected = valueString.substring(entry.getKey().length());
return entry.getValue().apply(path, parseLocalDateTime(expected));
}
}
return new FieldHasExactValueAsserter(path, value);
}However, I thought to myself, I can do better, so I put the size asserter If/Else statements into their own method, the null/empty array If/else statements into their own and got that whole For loop down to...
for (Map.Entry<String, Object> entry : createFieldsKeyValuesMap.entrySet()) {
String path = entry.getKey();
Object value = entry.getValue();
JsonAsserter asserter = getNullOrEmptyAsserter(value, path);
if (asserter == null && path.endsWith(ASSERT_PATH_SIZE)) {
path = path.substring(0, path.length() - ASSERT_PATH_SIZE.length());
asserter = getPathSizeAsserter(path, value);
} else if (asserter == null && value instanceof String) {
asserter = getStringAsserter(value, path);
} else if (asserter == null) {
asserter = new FieldHasExactValueAsserter(path, value);
}
asserters.add(asserter);
}and
protected JsonAsserter getNullOrEmptyAsserter(Object value, String path) {
Map<String, Function<String, JsonAsserter>> nullEmptyAsserters = new HashMap<>();
nullEmptyAsserters.put(ASSERT_VALUE_NOT_NULL, FieldIsNotNullAsserter::new);
nullEmptyAsserters.put(ASSERT_VALUE_IS_NOT_NULL, FieldIsNotNullAsserter::new);
nullEmptyAsserters.put(ASSERT_VALUE_IS_NULL, FieldIsNullAsserter::new);
nullEmptyAsserters.put(ASSERT_VALUE_NULL, FieldIsNullAsserter::new);
nullEmptyAsserters.put(ASSERT_VALUE_EMPTY_ARRAY, ArrayIsEmptyAsserterImpl::new);
for (Map.Entry<String, Function<String, JsonAsserter>> mapEntry : nullEmptyAsserters.entrySet()) {
if (mapEntry.getKey().equals(value)) {
return mapEntry.getValue().apply(path);
}
}
return null;
}
protected JsonAsserter getPathSizeAsserter(String path, Object value) {
if (value instanceof Number) {
return new ArraySizeAsserterImpl(path, (Integer) value);
} else if (value instanceof String) {
return new ArraySizeAsserterImpl(path, (String) value);
} else {
throw new RuntimeException(format("Oops! Unsupported value for .SIZE: %s", value));
}
}in addition to the extracted methods you saw above.
This method was trying to be too careful with its Try/Catch statements and ended up duplicating code in the process, as shown below...
public List<Step> createFromStepFile(Step thisStep, String stepId) {
List<Step> thisSteps = new ArrayList<>();
if (thisStep.getStepFile() != null) {
try {
thisSteps.add(objectMapper.treeToValue(thisStep.getStepFile(), Step.class));
} catch (JsonProcessingException e) {
LOGGER.error("\n### Error while parsing for stepId - {}, stepFile - {}",
stepId, thisStep.getStepFile());
throw new RuntimeException(e);
}
} else if(null != thisStep.getStepFiles() && !thisStep.getStepFiles().isEmpty()) {
try {
for(int i = 0; i < thisStep.getStepFiles().size(); i++)
thisSteps.add(objectMapper.treeToValue(thisStep.getStepFiles().get(i), Step.class));
} catch (JsonProcessingException e) {
LOGGER.error("\n### Error while parsing for stepId - {}, stepFile - {}",
stepId, thisStep.getStepFiles());
throw new RuntimeException(e);
}
}
return thisSteps;I moved the Try/Catch statements around to encompass the whole If/Else, instead of the other way around, and removed the duplicate code in the process.
public List<Step> createFromStepFile(Step thisStep, String stepId) {
List<Step> thisSteps = new ArrayList<>();
String branch = "";
try {
if (thisStep.getStepFile() != null) {
branch = "A";
thisSteps.add(objectMapper.treeToValue(thisStep.getStepFile(), Step.class));
} else if(null != thisStep.getStepFiles() && !thisStep.getStepFiles().isEmpty()) {
branch = "B";
for(int i = 0; i < thisStep.getStepFiles().size(); i++)
thisSteps.add(objectMapper.treeToValue(thisStep.getStepFiles().get(i), Step.class));
}
} catch (JsonProcessingException e) {
LOGGER.error("\n### Error while parsing for stepId - {}, stepFile - {}, branch - {}",
stepId, thisStep.getStepFiles(), branch);
throw new RuntimeException(e);
}
return thisSteps;
}I had a hard time finding anything with this code I could tweak to make it better...
I reversed the inline conditional statement to make the processing easier. Forcing the processor to flip the boolean is just one more cycle that could be used elsewhere.
return !hasHeader ? null : Arrays.stream(parsedHeaderLine).map(s -> s.substring(1,s.length()-1)).toArray(String[]::new);became...
return hasHeader ? Arrays.stream(parsedHeaderLine).map(s -> s.substring(1, s.length() - 1)).toArray(String[]::new) : null;By moving the valuesMap initialization down a line, I was able to use the length of the parsedLine variable to create a Map of the correct capacity rather than relying on the default size. I was also able to remove a redundant .toString() call.
private Map<String, Object> resolveCsvLine(String csvLine, String[] headers) {
Map<String, Object> valuesMap = new HashMap<>();
String[] parsedLine = csvParser.parseLine(csvLine + LINE_SEPARATOR);
IntStream.range(0, parsedLine.length).forEach(i -> valuesMap.put(i + "", parsedLine[i]));
if (headers != null){
IntStream.range(0, headers.length).forEach(i -> {
if(!headers[i].contains(" ") && !headers[i].isEmpty()){
valuesMap.put("PARAM."+headers[i], TokenUtils.resolveKnownTokens(parsedLine[i]).toString());
}
});
}became...
private Map<String, Object> resolveCsvLine(String csvLine, String[] headers) {
String[] parsedLine = csvParser.parseLine(csvLine + LINE_SEPARATOR);
Map<String, Object> valuesMap = new HashMap<>(parsedLine.length);
IntStream.range(0, parsedLine.length).forEach(i -> valuesMap.put(i + "", parsedLine[i]));
if (headers != null){
IntStream.range(0, headers.length).forEach(i -> {
if(!headers[i].contains(" ") && !headers[i].isEmpty()){
valuesMap.put("PARAM."+headers[i], TokenUtils.resolveKnownTokens(parsedLine[i])); // .toString());
}
});
}I added ZeroCodeAssertionsProcessorImplConvertJsonTypeToJavaTypeTest.java, ZeroCodeAssertionsProcessorImplDateTimeAsserterFinderTest.java, ZeroCodeAssertionsProcessorImplNullOrEmptyArrayAsserterTest.java, ZeroCodeAssertionsProcessorImplNumberValueAssertersTest.java, ZeroCodeAssertionsProcessorImplSizeAsserterFinderTest.java, and ZeroCodeAssertionsProcessorImplStringValueAssertersTest.java to test the new methods that were created in ZeroCodeAssertionsProcessorImpl.java.
I added testGetResolvedScenarioState(), testGetStepStates(), testGetMissingExecutedStep(), and setScenarioStateTemplate() to increase test coverage.
I reversed the expected and actual arguments of two .assertEquals() calls to bring them into compliance with what the method expects.
I suppressed the "unchecked casts" warnings and replaced the calls to the deprecated org.junit.Assert.assertThat() method with current code. This resulted in my also stripping out all direct use of the org.hamcrest library. I was also able to remove a call to the deprecated org.junit.rules.ExpectedException.none() method, which resulted in being able to remove the import statement for the org.junit.Rule annotation.
I made the IDE happier by using the final keyword on the class's instance of the class under testing. I also removed several references to assertThat() and replaced them with org.junit.Assert method calls.
I removed the expected RuntimeException from the Test annotation and put in an assertion instead.
I removed another call to org.junit.rules.ExpectedException.none() and replaced the impacted code with an org.junit.Assert.assertThrows() call.
I removed two more assertThat() calls.