Skip to content

Conversation

@darkspirit510
Copy link
Contributor

This is just an idea about a refactoring, it does not work (for now). Just moved some lines around and created new classes.

Reason: It feels strange to have the selection of tool within BenchmarkScore.java, but other stuff seperated in several classes. Moreover, BenchmarkScore.java is almost 2k lines long. I had the idea to create a wrapper class to access a result file (including JSON/XML/String/Whatever access) and use some kind of marker (in this case abstract superclass, could be annotation/interface, too) and just ask each instance if it is the correct reader for a given file.

@darkspirit510
Copy link
Contributor Author

@darkspirit510
Copy link
Contributor Author

PS: I'd prefer to just create classes without having to touch BenchmarkScore and do crazy things like:

        readers.add(new HorusecReader());
        readers.add(new InsiderReader());
        readers.add(new ShiftLeftScanReader());
        readers.add(new WapitiJsonReader());
        readers.add(new ZapJsonReader());

But I didn't find a solution I like...

@davewichers
Copy link
Contributor

Some of the readers provide methods BenchmarkScore can call that determine if a specific file matches for that tool. I can't recall the method name but I'm sure you can find example. The thing I like about that pattern is the logic for results file matching is in the parser for the tool, rather than in BenchmarkScore itself. Maybe that pattern can help some?

@darkspirit510
Copy link
Contributor Author

Yes, all the readers I contributed (Horusec, Insider, ShiftLeftScan, Wapiti, ZaqJson) come with such method, this is just a step further and getting rid of the if-chain in BenchmarkScore. Just iterate over a collection of Readers and find the one (if any) that is capable of reading a given file.

@darkspirit510
Copy link
Contributor Author

Migrated all Readers (except FortifyReader, will follow soon). Reduced/randomized some SAST testdata to be used in JUnit tests. Not sure about how to get result files of all the commercial tools...

@darkspirit510 darkspirit510 mentioned this pull request Jan 25, 2022
52 tasks
@darkspirit510 darkspirit510 changed the title WIP / RFC: refactoring attempt Check for matching Reader within reader and not in BenchmarkScore (+ lots of tests) Jan 25, 2022
@darkspirit510 darkspirit510 marked this pull request as ready for review January 25, 2022 21:38
@davewichers davewichers merged commit fb3a4f0 into OWASP-Benchmark:main Feb 2, 2022
@darkspirit510 darkspirit510 deleted the rfc-refactoring branch February 2, 2022 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants