Skip to content

Commit e1a6cb1

Browse files
dmaccormackdmaccormack
committed
update: fix some code smells
Note, this commit changes ParseException's interface to comply with the principle that Exception instances should be immutable. It also changes Parser's interface to accept a Set rather than a EnumSet. Remove the literal "false" boolean value. https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj06IRhxtzbnQIFCT&open=AXFhj06IRhxtzbnQIFCT Replace the type specification in this constructor call with the diamond operator ("<>"). https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj06YRhxtzbnQIFCV&open=AXFhj06YRhxtzbnQIFCV https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj06YRhxtzbnQIFCW&open=AXFhj06YRhxtzbnQIFCW Add a private constructor to hide the implicit public one. https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj06oRhxtzbnQIFCc&open=AXFhj06oRhxtzbnQIFCc This block of commented-out lines of code should be removed. https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXIWePgqqmOR1dUOS17q&open=AXIWePgqqmOR1dUOS17q https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj06oRhxtzbnQIFCh&open=AXFhj06oRhxtzbnQIFCh https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXIWePfZqmOR1dUOS17p&open=AXIWePfZqmOR1dUOS17p https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj08yRhxtzbnQIFCj&open=AXFhj08yRhxtzbnQIFCj Make this "position" field final. https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj05ORhxtzbnQIFB_&open=AXFhj05ORhxtzbnQIFB_ Rename "position" which hides the field declared at line 33. https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXFhj05ORhxtzbnQIFCB&open=AXFhj05ORhxtzbnQIFCB Type to an interface rather than the implementation https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXIWeParqmOR1dUOS17k&open=AXIWeParqmOR1dUOS17k https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXIWePckqmOR1dUOS17n&open=AXIWePckqmOR1dUOS17n Refactor the code of this assertThrows to have only one invocation throwing an exception. https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXJYdgtsYcuZji4UbJ3Z&open=AXJYdgtsYcuZji4UbJ3Z https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXJYdgtsYcuZji4UbJ3a&open=AXJYdgtsYcuZji4UbJ3a https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXJYdgtsYcuZji4UbJ3b&open=AXJYdgtsYcuZji4UbJ3b https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXJYdgtsYcuZji4UbJ3c&open=AXJYdgtsYcuZji4UbJ3c https://sonarcloud.io/project/issues?id=jsonurl_jsonurl-java&issues=AXJYdgtsYcuZji4UbJ3d&open=AXJYdgtsYcuZji4UbJ3d
1 parent 2ef47ac commit e1a6cb1

File tree

12 files changed

+124
-75
lines changed

12 files changed

+124
-75
lines changed

‎module/jsonurl-core/src/main/java/org/jsonurl/CharUtil.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ final class CharUtil {
163163
}
164164

165165
static final boolean isDigit(char c) {
166-
return c > 127 ? false : ((CHARBITS[c] & IS_DIGIT) != 0);
166+
return c <= 127 && ((CHARBITS[c] & IS_DIGIT) != 0);
167167
}
168168

169169
static final int hexDecode(char c) {

‎module/jsonurl-core/src/main/java/org/jsonurl/JavaValueFactory.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@
1919

2020
import java.util.ArrayList;
2121
import java.util.Collections;
22-
import java.util.EnumSet;
2322
import java.util.HashMap;
2423
import java.util.List;
2524
import java.util.Map;
25+
import java.util.Set;
2626

2727
/**
2828
* A {@link org.jsonurl.ValueFactory ValueFactory} based on J2SE8 data types.
@@ -95,12 +95,12 @@ default Object getNull() {
9595

9696
@Override
9797
default List<Object> newArrayBuilder() {
98-
return new ArrayList<Object>(4);
98+
return new ArrayList<>(4);
9999
}
100100

101101
@Override
102102
default Map<String,Object> newObjectBuilder() {
103-
return new HashMap<String,Object>(4);
103+
return new HashMap<>(4);
104104
}
105105

106106
@Override
@@ -138,7 +138,7 @@ default String getString(String s) {
138138
}
139139

140140
@Override
141-
default boolean isValid(EnumSet<ValueType> types, Object value) {
141+
default boolean isValid(Set<ValueType> types, Object value) {
142142
if (isNull(value)) {
143143
return types.contains(ValueType.NULL);
144144
}

‎module/jsonurl-core/src/main/java/org/jsonurl/JsonUrl.java‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,16 @@ public final class JsonUrl {
4949
* a namespace to hide private, parse-specific static fields and methods.
5050
*/
5151
static final class Parse { //NOPMD - internal utility class
52+
53+
private Parse() {
54+
}
5255

5356
private static final int percentDecode(
5457
CharSequence s,
5558
int off,
5659
int len) {
5760

5861
if (off + 2 > len) {
59-
// return 0;
6062
throw new SyntaxException(ERR_MSG_BADPCTENC, off);
6163
}
6264

@@ -190,12 +192,14 @@ private static final String string(
190192
} else if ((b & 0xf8) == 0xf0) { // 11110xxx (yields 3 bits)
191193
sumb = b & 0x07;
192194
more = 3; // Expect 3 more bytes
193-
} else if ((b & 0xfc) == 0xf8) { // 111110xx (yields 2 bits)
194-
sumb = b & 0x03;
195-
more = 4; // Expect 4 more bytes
196-
} else /*if ((b & 0xfe) == 0xfc)*/ { // 1111110x (yields 1 bit)
197-
sumb = b & 0x01;
198-
more = 5; // Expect 5 more bytes
195+
} else {
196+
// per rfc3629 everything else is invalid.
197+
//
198+
// Ideally I'd throw a MalformedInputException, but
199+
// I want to throw something that extends RuntimeException
200+
// and it does not.
201+
//
202+
throw new IllegalArgumentException("utf-8 decode error");
199203
}
200204
}
201205
if (needEndQuote) {

‎module/jsonurl-core/src/main/java/org/jsonurl/ParseException.java‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public class ParseException extends RuntimeException {
3030
/**
3131
* the position in the input text.
3232
*/
33-
private int position;
33+
private final int position;
3434

3535
/**
3636
* Create a new ParseException.
@@ -54,13 +54,6 @@ public int getPosition() {
5454
return position;
5555
}
5656

57-
/**
58-
* Set the position of the exception.
59-
*/
60-
public void setPosition(int position) {
61-
this.position = position;
62-
}
63-
6457
/**
6558
* Exception type description.
6659
*/
@@ -77,9 +70,9 @@ public String toString() {
7770
.append(": ")
7871
.append(getMessage());
7972

80-
int position = getPosition();
81-
if (position > -1) {
82-
sb.append(" at ").append(position);
73+
int p = getPosition();
74+
if (p > -1) {
75+
sb.append(" at ").append(p);
8376
}
8477

8578
return sb.toString();

‎module/jsonurl-core/src/main/java/org/jsonurl/Parser.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.Deque;
3535
import java.util.EnumSet;
3636
import java.util.LinkedList;
37+
import java.util.Set;
3738

3839
/**
3940
* A JSON-&gt;URL parser.
@@ -259,7 +260,7 @@ public V parse(CharSequence s, int off, int length) {
259260

260261
/**
261262
* Parse a character sequence. Simple calls
262-
* {@link #parse(CharSequence, int, int, EnumSet)
263+
* {@link #parse(CharSequence, int, int, Set)
263264
* parse(s, off, length, EnumSet.of(canReturn))}.
264265
*/
265266
public V parse(CharSequence s, int off, int length, ValueType canReturn) {
@@ -268,7 +269,7 @@ public V parse(CharSequence s, int off, int length, ValueType canReturn) {
268269

269270
/**
270271
* Parse a character sequence. Simple calls
271-
* {@link #parse(CharSequence, int, int, EnumSet)
272+
* {@link #parse(CharSequence, int, int, Set)
272273
* parse(s, off, length, EnumSet.of(canReturn))}.
273274
*/
274275
public V parse(CharSequence s, ValueType canReturn) {
@@ -289,7 +290,7 @@ public V parse(
289290
CharSequence s,
290291
int off,
291292
int length,
292-
EnumSet<ValueType> canReturn) {
293+
Set<ValueType> canReturn) {
293294

294295
if (length == 0) {
295296
throw new SyntaxException(ERR_MSG_NOTEXT, 0);
@@ -342,7 +343,9 @@ public V parse(
342343

343344
stateStack.push(State.PAREN);
344345

345-
for (int pos = off + 1;;) {
346+
int pos = off + 1;
347+
348+
for (;;) {
346349
if (pos == stop) {
347350
throw new SyntaxException(ERR_MSG_STILLOPEN, pos);
348351
}

‎module/jsonurl-core/src/main/java/org/jsonurl/ValueFactory.java‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package org.jsonurl;
22

33
import java.util.EnumSet;
4+
import java.util.Set;
45

56
/*
67
* Copyright 2019 David MacCormack
@@ -220,7 +221,7 @@ default B getBoolean(boolean b) {
220221

221222
/**
222223
* Test if the given {@code value} has the given {@code type}.
223-
* This simply calls {@link #isValid(EnumSet, Object)
224+
* This simply calls {@link #isValid(Set, Object)
224225
* isValid(EnumSet.of(type), value)}.
225226
* @param type allowed type
226227
* @param value value to test
@@ -235,7 +236,7 @@ default boolean isValid(ValueType type, V value) {
235236
* @param value value to test
236237
* @return true if the type of value is in the given types set.
237238
*/
238-
public boolean isValid(EnumSet<ValueType> types, V value);
239+
public boolean isValid(Set<ValueType> types, V value);
239240

240241
/**
241242
* Test if the given object is the empty value.
@@ -256,18 +257,6 @@ default boolean isEmpty(Object obj) {
256257
default boolean isNull(Object obj) {
257258
return obj == getNull();
258259
}
259-
260-
/*
261-
* Get a true, false, or null value for the given text. This simply calls
262-
* {@link #getTrueFalseNull(CharSequence, int, int)
263-
* getTrueFalseNull(s, 0, s.length())}.
264-
*
265-
* @param s the text
266-
* @return a valid value or null
267-
*
268-
default V getTrueFalseNull(CharSequence s) {
269-
return getTrueFalseNull(s, 0, s.length());
270-
}*/
271260

272261
/**
273262
* get a true, false, or null value for the given text.

‎module/jsonurl-core/src/main/java/org/jsonurl/ValueType.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
package org.jsonurl;
22

3-
import java.util.EnumSet;
3+
import java.util.Set;
44

55
/**
66
* An enumeration of JSON&gt;URL value types.
@@ -54,7 +54,7 @@ public boolean isCompositeOrNull() {
5454
* Test if the given EnumSet contains a composite value.
5555
* @param set the set to test
5656
*/
57-
public static final boolean containsComposite(EnumSet<ValueType> set) {
57+
public static final boolean containsComposite(Set<ValueType> set) {
5858
return set.contains(OBJECT) || set.contains(ARRAY);
5959
}
6060
}

‎module/jsonurl-core/src/test/java/org/jsonurl/AbstractParseTest.java‎

Lines changed: 49 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -360,14 +360,12 @@ void testStringsWithStructCharsQuotedAndEncoded(String s)
360360
@Tag("autostring")
361361
@ValueSource(strings = {
362362
"hello",
363-
"Bob's House",
364363
"t", "tr", "tru", "True", "tRue", "trUe", "truE",
365364
"f", "fa", "fal", "fals", "False", "fAlse", "faLse", "falSe", "falsE",
366365
"n", "nu", "nul", "Null", "nUll", "nuLl", "nulL",
367366
})
368-
void testString(String s) throws UnsupportedEncodingException {
369-
String in = urlEncode(s).replace("%27", "'");
370-
parse(ValueType.STRING, in, getFactoryString(s));
367+
void testString(String s) throws IOException {
368+
parse(ValueType.STRING, s, getFactoryString(s));
371369
}
372370

373371
private S getFactoryString(String s) {
@@ -425,9 +423,10 @@ void testMisc() {
425423
"(a:b,'c'd)",
426424
})
427425
void testSyntaxException(String s) throws ParseException {
426+
Parser<?, ?, ?, ?, ?, ?, ?, ?, ?, ?> p = newParser();
428427
assertThrows(
429428
SyntaxException.class,
430-
() -> newParser().parse(s));
429+
() -> p.parse(s));
431430
}
432431

433432
@ParameterizedTest
@@ -439,39 +438,61 @@ void testSyntaxException(String s) throws ParseException {
439438
"'(1,2)', STRING",
440439
})
441440
void testSyntaxException2(String text, String type) throws ParseException {
441+
Parser<?, ?, ?, ?, ?, ?, ?, ?, ?, ?> p = newParser();
442442
assertThrows(
443443
SyntaxException.class,
444-
() -> newParser().parse(text, ValueType.valueOf(type)));
444+
() -> p.parse(text, ValueType.valueOf(type)));
445445
}
446446

447+
@ParameterizedTest
448+
@Tag("parse")
449+
@Tag("exception")
450+
@ValueSource(strings = {
451+
"%FA%80%80%80%80", //0x200000
452+
})
453+
void testUtf8EncodingException(String text) throws ParseException {
454+
Parser<?, ?, ?, ?, ?, ?, ?, ?, ?, ?> p = newParser();
455+
assertThrows(
456+
IllegalArgumentException.class,
457+
() -> p.parse(text));
458+
}
447459

448460
@Test
449461
@Tag("parse")
450462
@Tag("exception")
451463
void testException() throws ParseException {
452-
assertThrows(
453-
LimitException.class,
454-
() -> {
455-
Parser<?,?,?,?,?,?,?,?,?,?> p = newParser();
456-
p.setMaxParseChars(2);
457-
p.parse("true");
458-
});
464+
{
465+
Parser<?,?,?,?,?,?,?,?,?,?> p = newParser();
466+
p.setMaxParseChars(2);
467+
468+
assertThrows(
469+
LimitException.class,
470+
() -> {
471+
p.parse("true");
472+
});
473+
}
459474

460-
assertThrows(
461-
LimitException.class,
462-
() -> {
463-
Parser<?,?,?,?,?,?,?,?,?,?> p = newParser();
464-
p.setMaxParseDepth(2);
465-
p.parse("(((1)))");
466-
});
467-
468-
assertThrows(
469-
LimitException.class,
470-
() -> {
471-
Parser<?,?,?,?,?,?,?,?,?,?> p = newParser();
472-
p.setMaxParseValues(2);
473-
p.parse("(1,2,3)");
474-
});
475+
{
476+
Parser<?,?,?,?,?,?,?,?,?,?> p = newParser();
477+
p.setMaxParseDepth(2);
478+
479+
assertThrows(
480+
LimitException.class,
481+
() -> {
482+
p.parse("(((1)))");
483+
});
484+
}
485+
486+
{
487+
Parser<?,?,?,?,?,?,?,?,?,?> p = newParser();
488+
p.setMaxParseValues(2);
489+
490+
assertThrows(
491+
LimitException.class,
492+
() -> {
493+
p.parse("(1,2,3)");
494+
});
495+
}
475496

476497
ParseException pe = null;
477498
try {

‎module/jsonurl-core/src/test/java/org/jsonurl/JavaValueFactoryParseTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ protected String getString(String key, Map<String,Object> value) {
113113
}
114114

115115
@Test
116+
@Override
116117
void testMisc() {
117118
super.testMisc();
118119

0 commit comments

Comments
 (0)