SMS Parsing

[ricdezen]

The parser and therefore the strategy is called only right before the call to SMSCore, which means the SMSMessage length check is rendered useless, because parseData just takes the String and adds the header, while parseMessage removes the header and builds an SMSMessage. Shouldn't it be the opposite?

[CremaLuca]

So you're saying that we should check the message length right before calling the SMSCore? You're right, we should think of a way to have the parser check for maximum length or set a maximum message content length because this parameter depends on the strategy.

[ricdezen]

Yes, I mean, if the message is at exactly max length when being constructed and the parser is then called to actually send it, the header is added and causes it to overflow the length limit

[giovannivelludo]

I'll write down my question here since I don't want to open another issue on parsing:

Why are we including parsing of messages in smslibrary? The only purpose of this library is sending and receiving messages to and from certain peers, and I remember professor Peserico telling us that if we don't have a reason to include some feature at a low level, we should let higher levels handle that. In our case higher levels would be our killer-app or our dictionary, which would use smslibrary just to receive and send messages.
An app using this library might not want to parse the content of SMS messages, the same way it might not want to use PreferencesManager (that's why it wasn't included in this library).
Not only that, but class SMSMessageHandler is 101 lines of code and the only thing it does is adding a character at the beginning of messages to be sent and removing it from messages arriving.

Also, Unicode char 0x02 used in DefaultSMSMessageParseStrategy.HIDDEN_CHARACTER is not part of the GSM character set and thus it will reduce the length of our SMS messages to 70 characters, instead of the 160 available when using only characters from the GSM character set.

Finally, in the documentation of SMSMessageHandler I'm reading expressions like "parses to" or "parses into", but there's no such thing as "parsing to" or "parsing into". Parsing a String means analyzing it and finding the tokens we need.

[ricdezen]

As far as I can tell "parsing" has been there since day one to distinguish sms messages sent by our library from messages coming from other sources, we just didn't give it a name, are we supposed to get rid of it all? If we were to literally only send and receive messages we could trim the library down to four classes. Besides, having introduced the ability to set a parsing strategy the user is able to quickly set the strategy to just "send messages with no header and receive any message" with like two lines of code.

[ricdezen]

As a matter of fact, we could change the default strategy to exactly that, and let the user add a header or whatever else if he wishes

[giovannivelludo]

As a matter of fact, we could change the default strategy to exactly that, and let the user add a header or whatever else if he wishes

I agree, deciding if and what header to use should be done by the application using the library

[ricdezen]

Well, then if we want to let the user set the strategy easily, the parsing procedure should remain imo, the issue of getting it to behave properly remains

[ricdezen]

About the hidden character and the grammar error with the parsing verb you're right about both

[giovannivelludo]

@ricdezen

As far as I can tell "parsing" has been there since day one to distinguish sms messages sent by our library from messages coming from other sources, we just didn't give it a name, are we supposed to get rid of it all?

@albertoursino pointed out in Cogno-Marco/killer-app#18 (comment) that we need a check for a special character in the app too, in order to differentiate between messages sent to different apps using the same library and installed on the same phone (killer-app and network-dictionary), which makes a check in the library pointless. Technically the check is used by the application by implementing its own MessageParseStrategy, and then calling SMSMessageHandler's methods, but it's easier and quicker to just do the parsing in a function in the app.

EDIT: nevermind, a class for parsing being present in the library doesn't prevent people from doing whatever they want in the app. I just think the default strategy shouldn't check for the special character.

[giovannivelludo]

Regarding the maximum length of messages:

I think SMSMessage.checkMessageText() should check if the (length of the SMS + length of DefaultSMSParseStrategy.HIDDEN_CHARACTER) fits SMSMessage.MAX_MSG_TEXT_LEN.
However, if whoever is using this library creates a new SMSMessage and then uses SMSMessageParser.getInstance().setMessageParseStrategy() to change how many characters are added in the beginning of the message, then the checks on the validity of SMSMessages already created might be wrong, for example if the hidden characters become longer in the new strategy.

Doing the check in getSMSContent() (or in general after the SMSMessage is created correctly) would not be a good idea, because then the user won't know why their message was created correctly but then failed a second check on its length.

A simple solution could be removing class SMSMessageParser and letting the app using this library decide if and what character to insert at the beginning of messages. That class was meant to make it easier for different groups to decide how they wanted to identify messages meant for their app, but in the end it's unnecessarily complicating everything.
A worse solution would be letting custom MessageParseStrategy classes only use one character at the beginning of messages, because at that point we might as well remove the possibility of creating those custom strategies, since it's faster to just have a method to change the character that is added before the message.
In both cases the best thing to do is letting developers of the app using this library choose if and how they want to add some ID or password in the beginning of each message, as it was done in our killer app.

[giovannivelludo]

@CremaLuca this is a bug, could you set the label appropriately?