next: Support more sgf properties#376
Conversation
OlivierBlanvillain
force-pushed
the
next_sgfprops
branch
from
60a86f4 to
c8fa385
Compare
|
Ok, I'll update all unmerge code. |
|
@zsalch I won't have time to give you a detailed review today (it's coming next ^^'), but I can already tell you that I would like to see some tests for the additional sgf properties you want to support. |
|
@OlivierBlanvillain |
OlivierBlanvillain
left a comment
OlivierBlanvillain
left a comment
There was a problem hiding this comment.
Nice work @zsalch!
| } | ||
|
|
||
| public static boolean isListProperty(String key) { | ||
| for (String k : listProps) { |
There was a problem hiding this comment.
I think this is the same as listProps.contains(key). Given that it's so short you might want to remove the method altogether and simply do listProps.contains(key) instead...
Same for isMarkupProperty below.
There was a problem hiding this comment.
The listProps is a String[]. It seems no contains function.
There was a problem hiding this comment.
Indeed... What about 'Arrays.asList(listProps).contains(key)`?
There was a problem hiding this comment.
I think directly loop search should be faster.
There was a problem hiding this comment.
It's not :)
https://gist.github.com/OlivierBlanvillain/266d5176d559a66e47973f321162c770
JVM performance is a complicated subject...
| moveY, | ||
| LizzieFrame.OpenSansRegularBase, | ||
| moves[1], | ||
| (float) (stoneRadius * 1.4), |
There was a problem hiding this comment.
double labelRadius = stoneRadius * 1.4;| (float) (stoneRadius * 1.4), | ||
| (int) (stoneRadius * 1.4)); | ||
| } else if ("TR".equals(key)) { | ||
| // Triangle |
There was a problem hiding this comment.
I think you can remove these comments, it's already clear enough from the method name, drawTriangle! (same for the calls below)
| public void reopen(int size) { | ||
| size = (size == 13 || size == 9) ? size : 19; | ||
| if (size != BOARD_SIZE) { | ||
| BOARD_SIZE = size; |
There was a problem hiding this comment.
If you mutate it here it really has to be lower cased: boardSize.
| * @return | ||
| */ | ||
| public static void addProperties(Map<String, String> props, String propsStr) { | ||
| if (propsStr != null) { |
There was a problem hiding this comment.
Same comment than above, can propsStr ever be null?
| StringBuilder tagBuilder = new StringBuilder(); | ||
| StringBuilder tagContentBuilder = new StringBuilder(); | ||
|
|
||
| for (int i = 0; i < propsStr.length(); i++) { |
There was a problem hiding this comment.
Looks very low level... Any reason not to use a regular expression?
There was a problem hiding this comment.
I haven't found a regular expression that can simple process sgf like following.
;CA[gb2312]AB[pe][pq][oq][nq][mq][cp][dq][eq][fp]AW[dc][cf][oc][qo][op][np][mp]
[ep][fq]AP[MultiGo:4.4.4]SZ[19]AB[qd]MULTIGOGM[1]
But maybe can change how to write the "KM[%s]PW[%s]PB[%s]DT[%s]AP[Lizzie: %s]".
| } | ||
|
|
||
| /** | ||
| * Add the properties |
There was a problem hiding this comment.
I think it might be useful do document that this done by mutating the props map, same comment for the other added methods in SGFParser.
|
LGTM, thanks! |
2 participants
Support more sgf properties, such as node properties (TR, SQ, CR, MA, LB, MN and AW/AB/AE of Move) and general properties.