Random SMSPeer generator. #49

ricdezen · 2020-02-10T12:26:42Z

Premise

This is not a class to be used at runtime, it's only useful for testing with consistent quantities of SMSPeers.
Due to special validity criteria of very few countries (Tavana, Norfolk Island, TA (?)) which I am still investigating, the class iterates until an appropriate number is generated.
Any other regions never returned invalid addresses after the first iteration, as long as I could tell.

Features

An SMSPeer generator that can generate numbers for any region, but is limited to 1000 for each, because it simply takes the example number from libphonenumber and changes the last three digits, in order not to mess up the national validity of the number.
I don't think there is any better logic for generating phone numbers without implementing the appropriate standards for every single country. If you do, please share.

Changes

Added RandomPeerGenerator as an interface.
Added RandomSMSPeerGenerator as an implementing class for SMSPeer.

Side Changes

Added SMSPeer.getDefaultRegion() because why in heavens would you want to just blindly set something.

I kept the classes in the main scope to avoid having to import the module with testImplementation when using Gradle, if you think moving it to the test scope is worth it, I'll do it.

Say something nice to who's going to review your code.
Something nice, also thanks for your time.

Pull from Remote

Implemented delivery report (Cogno-Marco#14)

They were reversed compared to the handler and it was bugging me immensely

* 'master' of https://github.com/Cogno-IDU/smslibrary: Feature: library working on app closed (Cogno-Marco#13) Created README.md instructions (Cogno-Marco#16)

# Conflicts: # smslibrary/src/main/java/com/eis/smslibrary/SMSMessageHandler.java # smslibrary/src/test/java/com/eis/smslibrary/SMSPeerTest.java

fix: added Nullable annotations. feat: added getDefaultRegion static method.

feat: added RandomPeerGenerator as an interface. feat: added RandomSMSPeerGenerator as a class to generate random SMSPeers.

…rmock bug.

giovannivelludo · 2020-02-13T19:29:06Z

smslibrary/src/main/java/com/eis/random/RandomPeerGenerator.java

+ * @param <P> the type of generated Peer.
+ * @author Riccardo De Zen.
+ */
+public interface RandomPeerGenerator<A, P extends Peer<A>> {


I'd move this class to the test folder since it's only useful in tests

Will do, I asked exactly because I couldn't decide, altough this means the library has to be imported as a test library too (I'm 99% sure of this, gotta check)

It's not a big deal since it's probably not compiled when the app is built, but I'd have to check how Proguard works to confirm it

giovannivelludo · 2020-02-13T19:30:00Z

smslibrary/src/main/java/com/eis/random/RandomSMSPeerGenerator.java

+ *
+ * @author Riccardo De Zen
+ */
+public class RandomSMSPeerGenerator implements RandomPeerGenerator<String, SMSPeer> {


I'd move this class to the test folder since it's only useful in tests

giovannivelludo · 2020-02-13T19:38:25Z

smslibrary/src/main/java/com/eis/smslibrary/SMSMessageHandler.java

         * @return the parsed SMSMessage if the string was correct, null otherwise
         */
        @Override
-        public SMSMessage parseMessage(@NonNull final String channelData, @NonNull final SMSPeer channelPeer) {
+        public SMSMessage parseMessage(@NonNull final String channelData,


Did you use a keyboard shortcut to automatically wrap the text in this file?

Yep, happened when pulling from the main repo because I had the reformat option on, did you prefer it like it was before?

No, I'd just like to know what shortcut you used

Just the usual CTRL+ALT+L

That's weird, it doesn't automatically wrap text when I use it, maybe I have to change something in my settings

I may have changed the settings but I've had Android Studio installed for around two years, so I can't really remember, sorry

giovannivelludo · 2020-02-13T19:49:15Z

smslibrary/src/main/java/com/eis/random/RandomSMSPeerGenerator.java

+     * Method to generate an invalid address. As a simple invalidity criteria, a random String is
+     * returned.


A random string is not always invalid, since you're returning a String with a predefined first character I would say that the first part of the String is not random, and that this is done to guarantee invalidity.

Yeah to make sure the address is invalid I added a header to the generated strings, I should change these specs

giovannivelludo · 2020-02-13T19:54:22Z

smslibrary/src/main/java/com/eis/random/RandomSMSPeerGenerator.java

+     * @return a random String, made of between 1 and 16 characters. The String always starts
+     * with {@link RandomSMSPeerGenerator#INVALID_ADDRESS_HEADER}.
+     */
+    private String randomString() {


It's not really random, the first part of the string is predetermined. I'd call it almostRandomString(), or partiallyRandomString(), or invalidRandomString().

giovannivelludo · 2020-02-13T19:55:52Z

smslibrary/src/main/java/com/eis/random/RandomSMSPeerGenerator.java

+                maxDigits = 15,
+                minDigits = 2,
+                maxCharValue = 255;
+        int digits = Math.abs(random.nextInt() % maxDigits) + minDigits;


The result could be bigger than maxDigits

Woops my bad I changed this a couple times and forgot to fix it

giovannivelludo · 2020-02-13T23:46:57Z

smslibrary/src/test/java/com/eis/smslibrary/RandomSMSPeerGeneratorTest.java

+@RunWith(Parameterized.class)
+public class RandomSMSPeerGeneratorTest {
+
+    private static final RandomSMSPeerGenerator GENERATOR = new RandomSMSPeerGenerator();


This is not a constant. It shouldn't be all upper-case.

Thanks for the reference document, I never really put much thought into this and assumed static final -> constant, but now I see, "all Object fields that are never followed by a dot", that is a fair point, method calls could modify the state of an Object and thus its value.
I can't probably track down all of my static final objects named like this, but I will fix at least this and avoid it in the future.

giovannivelludo · 2020-02-13T23:56:55Z

smslibrary/src/test/java/com/eis/smslibrary/RandomSMSPeerGeneratorTest.java

+    @Parameterized.Parameters(name = "{index}: {0}")
+    public static Collection<String> data() {
+        String[] supportedCountries = PhoneNumberUtil
+                .getInstance()
+                .getSupportedRegions()
+                .toArray(new String[0]);
+        return Arrays.asList(supportedCountries);
+    }


Nice! I heard about parametrized tests in class, but had no idea of what they were.

fix: moved classes to test folder fix: improvements to RandomSMSPeerGenerator specs fix: moved event validity logging in RandomSMSPeerGeneratorTest to a separate method

giovannivelludo · 2020-02-14T18:01:24Z

smslibrary/src/test/java/com/eis/smslibrary/random/RandomSMSPeerGenerator.java

+    @NonNull
+    @Override
+    public String generateInvalidAddress() {
+        return getInvalidRandomString();


You might as well move the body of getInvalidRandomString() in here since it's the only place where it's used.

I dunno, I feel like leaving it separated in case I need to modify the class later or do something in the address method that doesn't involve the String directly, feels cleaner this way to me

tonin-alessandra

Nice, I think this could be really useful for testing!

ricdezen and others added 11 commits December 1, 2019 23:04

Merge pull request #1 from Cogno-IDU/master

b59bb30

Pull from Remote

Merge pull request #2 from Cogno-IDU/master

99751d6

Implemented delivery report (Cogno-Marco#14)

Swapped arguments in MessageParseStrategy

0495368

They were reversed compared to the handler and it was bugging me immensely

Merge branch 'master' of https://github.com/Cogno-IDU/smslibrary

70b91e8

* 'master' of https://github.com/Cogno-IDU/smslibrary: Feature: library working on app closed (Cogno-Marco#13) Created README.md instructions (Cogno-Marco#16)

Merge branch 'master' of https://github.com/Cogno-IDU/smslibrary

8dd2f59

# Conflicts: # smslibrary/src/main/java/com/eis/smslibrary/SMSMessageHandler.java # smslibrary/src/test/java/com/eis/smslibrary/SMSPeerTest.java

fix: reverted param order back to original

6c63877

Expand for details:

d8d6df5

fix: added Nullable annotations. feat: added getDefaultRegion static method.

Expand for details:

9ffd884

feat: added RandomPeerGenerator as an interface. feat: added RandomSMSPeerGenerator as a class to generate random SMSPeers.

fix: Removed unused constant and reverted mockito version due to powe…

468d5da

…rmock bug.

fix: made test log more clear.

a72e75e

fix: Removed unused fields after Codacy check.

5120a56

giovannivelludo suggested changes Feb 14, 2020

View reviewed changes

Expand for details:

72496d8

fix: moved classes to test folder fix: improvements to RandomSMSPeerGenerator specs fix: moved event validity logging in RandomSMSPeerGeneratorTest to a separate method

giovannivelludo suggested changes Feb 14, 2020

View reviewed changes

giovannivelludo approved these changes Feb 15, 2020

View reviewed changes

tonin-alessandra approved these changes Feb 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Random SMSPeer generator. #49

Random SMSPeer generator. #49

ricdezen commented Feb 10, 2020 •

edited

Loading

giovannivelludo Feb 13, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 14, 2020

giovannivelludo Feb 13, 2020

giovannivelludo Feb 13, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 14, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 14, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 13, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 13, 2020

giovannivelludo Feb 13, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 13, 2020

ricdezen Feb 14, 2020

giovannivelludo Feb 13, 2020

giovannivelludo Feb 14, 2020

ricdezen Feb 14, 2020

tonin-alessandra left a comment

		* Method to generate an invalid address. As a simple invalidity criteria, a random String is
		* returned.

Random SMSPeer generator. #49

Are you sure you want to change the base?

Random SMSPeer generator. #49

Conversation

ricdezen commented Feb 10, 2020 • edited Loading

Premise

Features

Changes

Side Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonin-alessandra left a comment

Choose a reason for hiding this comment

ricdezen commented Feb 10, 2020 •

edited

Loading