Code review comment for lp:~tapaal-approx/tapaal/tapaal-approx-new

Revision history for this message
Thomas Stig Jacobsen (tsja10) wrote :

Regarding the exportToCSV method from which the lines 910-016 is from, we've used the style of code which was already used with the multiple appends, if the lines should be refactored, I think the rest of the method (or the class) should be refactored as well. A possible explanation to the way this is done now is that the XmlLoader when opening nets expects a certain number of delimiters for padding but I haven't check up on that. If not, I do agree this should be redone.

> Just a quick question about the following lines:
>
> 910 s.append((query != null) ?
> (query.useSymmetry() ? "Yes" : "No") : "");
> 911 s.append(DELIMITER);
> 912 s.append((query != null) ?
> getVerificationMethod(query) : "");
> 913 + s.append(DELIMITER);
> 914 + s.append((query != null) ?
> getApproximationMethod(query) : "");
> 915 + s.append(DELIMITER);
> 916 + s.append((query != null) ?
> query.approximationDenominator() : "");
>
> For me it seems more natural to make one if one query and then do all the
> append stuff:
>
> if (query != null) {
> //build string
> s.append(query.useSymmetry() ? "Yes" : "No");
> s.append(....)
> ...
> }
>
> The other way seams a little strange to me (unless there is something special
> about how query works, which really should be fixed then). (i know i was done
> this way in the original code as well, but i still think it needs to be fixed)

« Back to merge proposal