Collect: Put regex in meaningfully-named methods and introduce unit tests

[doomers]

See the discussion below for the suggested approach. The goal is to give meaning to the regex and test them.

[doomers]

@yanokwa @lognaturel Please have a look at this.

[dcbriccetti]

Hi. Is the point of this to reduce code duplication?

[doomers]

Yes, Sort of, Moreover it is highly recommended to keep all similar kind of stuffs in a separate class and access them from there.

[lognaturel]

I'm not sure that I love this as it hides behavior, there is very little (no?) duplication across classes and the regex are not related to each other other than by being regular expressions. I do agree that storing patterns in constants is helpful. I personally would prefer they were defined in the class they're used in or, if there are clusters with related functionality, perhaps separated in classes that way. @dcbriccetti I'm interested in your opinion on this too. I think the names of the constants should help explain what the pattern is (e.g. LETTER_START_NUMBER_REQUIRED instead of USERNAME).

Another, perhaps more controversial approach, would be to compose regex as recommended by Martin Fowler.

What I think would be most helpful is wrapping each regex in a small method with a meaningful name and putting those methods under unit tests to verify and document behavior. If that interests you, you could start by doing one or two, issuing a pull request so we can make sure we're all on the same page in terms of naming and testing strategy and then go from there!

[dcbriccetti]

I fully agree with the first paragraph. Less enthusiastic about the method-per-regex idea. Maybe part of the unit tests for a class would cover the regexes.

[lognaturel]

Is it possible to test a regex without it being in a method? I'm suggesting methods like setBinaryData in the BarcodeWidget class that encapsulate a single task and can be tested. (because many regex are currently buried in monster methods)

[dcbriccetti]

OK, I get you. That makes sense. I wasn’t keen on the idea of creating a new method for every regex specifically to enable testing. A method may use one or more regexes, and tests for the method might exercise them. Some operations using regexes might be abstracted into reusable methods, like isEmailValid.

[lognaturel]

Yes, sorry, I can see how my wording implied just wrapping the regexen. I mean creating methods that separate out meaningful chunks of work around each regex (or couple of regexen). isEmailValid is an good example of what I hope we can end up with. That would be a great way to start getting some pure JUnit tests in (that test important things).

[lognaturel]

@doomers What do you think?

[lognaturel]

@doomers Please let us know within the next day whether you'd like to address this. Otherwise we will let someone else take it.

[doomers]

@lognaturel sorry i am busy in my exams, if you wan't to assign it to someone else go ahead, would love to work to some other issues once i am done with my exams.

[SIMRAN1]

@lognaturel I want to work on this issue.Shall I proceed?

[mitchellsundt]

Just a caution on this ...

The Matcher class is not thread safe. It should be dynamically created as
needed. Pattern is thread safe.

http://stackoverflow.com/questions/1360113/is-java-regex-thread-safe

On Tue, Apr 4, 2017 at 4:46 AM, SIMRAN1 [e-mail] wrote:

I want to work on this issue.Shall I proceed?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/opendatakit/collect/issues/477#issuecomment-291475756,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACLO08SeIF5x-Rhvk7-vK8GE1SG840wTks5rsi2bgaJpZM4MPFob
.

--
Mitch Sundt
Software Engineer
University of Washington
[email protected]

[lognaturel]

Ah, thank you @mitchellsundt for that very good warning!

@SIMRAN1 sure! I would recommend you start by picking one class that has a buried and untested regex, thinking about a method that the regex would make sense within, extract that method and add a unit test. Then you can issue a pull request for that and we can discuss the approach. Does that sound good?

[SIMRAN1]

Thanks for the reply @lognaturel .I will solve this issue.But can you please give me example of it?

[lognaturel]

See this comment for an example of the kind of method we're talking about. For tests, you should be able to use straight junit.

[SIMRAN1]

@lognaturel I have send PR for this issue for GoogleSheetsAbstractUploader.Please Review it.

[breakbusyloop]

@opendatakit-bot claim