5
\$\begingroup\$

I am working to improve this script, which should prepare a text to be sent as a GSM short message using only the GSM 7-bit character coding with Basic Character Set Extension.

import logging
import traceback
import unicodedata
logger = logging.getLogger(__name__)

class IsGSM:

    def __init__(self):
        self.gsm = "@£$¥èéùìòÇ\nØø\rÅåΔ_ΦΓΛΩΠΨΣΘΞ\x1bÆæßÉ !\"#¤%&'()*+,-./0123456789:;<=>?" \
      "¡ABCDEFGHIJKLMNOPQRSTUVWXYZÄÖÑܧ¿abcdefghijklmnopqrstuvwxyzäöñüà"
        self.ext = "^{}\\[~]|€"
        self.toBeNormalized = '\xa0'+'\xc2'
        self.ascii_gsm = {}
        self.ascii_map()
        self.normalize_dict = {}
        self.normalize_map()

    def ascii_map(self):
        for c in self.gsm:
            #if ord(c) < 256:
            self.ascii_gsm[ord(c)] = True
        for c in self.ext:
            #if ord(c) < 256:
            self.ascii_gsm[ord(c)] = True

    def normalize_map(self):
        for c in self.toBeNormalized:
            self.normalize_dict[c] = True

    def _is_gsm(self, content):

        template = content

        try:
            template.encode('windows-1252')
        except UnicodeEncodeError:
            return False

        for c in content:
            if ord(c) not in self.ascii_gsm:
                return False
        return True

    @classmethod
    def is_gsm(cls, content):
        instance = IsGSM.instance()
        return instance._is_gsm(content)

    @classmethod
    def normalizeBodyContent(cls, content):
        instance = IsGSM.instance()
        return instance._normalizeBodyContent(content)

    @classmethod
    def instance(cls):
        """Singleton like accessor to instantiate backend object"""
        if not hasattr(cls, "_instance"):
            cls._instance = cls()
        return cls._instance

    def _normalizeBodyContent(self, content):
        try:
            target_content = [c for c in content]
            content_length = len(target_content)
            for ith in range(content_length):
                if target_content[ith] in self.normalize_dict:
                    target_content[ith] = unicodedata.normalize("NFKD",target_content[ith])

            content = "".join(target_content)
        except Exception as e:
               logger.error('Exception occurred normalizing content {0}'.format(content))
               traceback.print_exc()
               raise e
        return content

if __name__ == "__main__":
    print(IsGSM.is_gsm("Est a punto de restablecer la contrasea de su cartera de Kutxabank. Por favor, introduzca la sigui..."))
    print(IsGSM.is_gsm("This should fix the problem. Your content xxxxx from ÄA K COMPANY LTD will be delivered by xxxx today.Track your content at  some_url@&!"))

I can refactor the _normalizeBodyContent method like this:

def _normalizeBodyContent(self, content):
    try:
        target_content = [c for c in content]
        content_length = len(target_content)
        for ith in self.normalize_dict:
            target_content = target_content.replace(ith,
                                         unicodedata.normalize("NFKD", ith))
        content = "".join(target_content)
    except Exception as e:
        logger.error('Exception occurred normalizing content {0}'.format(content))
        traceback.print_exc()
        raise e
    return content

Is this an improvement on the original, in-terms of processing time and readability?

I'm particularly interested in comparing the lookup via in and overwrite against using the replace() library function. Review on any portion of the whole script is also appreciated.

\$\endgroup\$
7
  • \$\begingroup\$ @TobySpeight Edited, I am just wanting to make it sure that the script follows the standard best practices. Thanks \$\endgroup\$ Feb 1 '18 at 10:10
  • 2
    \$\begingroup\$ Title and indentation edited, hopefully it looks okay now to be reviewed \$\endgroup\$ Feb 1 '18 at 11:31
  • 2
    \$\begingroup\$ I've added some description showing what I think the code does - please check that I've understood correctly! \$\endgroup\$ Feb 1 '18 at 15:46
  • \$\begingroup\$ It looks like the normalization operation only performs Unicode compatibility decomposition for U+00A0 (NO-BREAK SPACE) and U+00C2 (LATIN CAPITAL LETTER A WITH CIRCUMFLEX). Why only those two characters? \$\endgroup\$ Feb 1 '18 at 17:57
  • 2
    \$\begingroup\$ What does the Windows-1252 character set have to do with GSM? \$\endgroup\$ Feb 1 '18 at 18:00
4
\$\begingroup\$

1. Review

  1. Normally, a class is used to implement a collection of objects with similar behaviour, and an object is used to implement some kind of persistent thing or data structure. But what kind of thing is an instance of the IsGsm class? It doesn't look as though it corresponds to any kind of thing.

    What you really have here is a function (IsGsm.is_gsm) that determines if text can be encoded in GSM 03.38. It's clear that this is a function rather than a method, because it does not use its cls argument. So it would be clearer and simpler if this were a function. In Python, not everything needs to be part of a class!

    See Jack Diederich's talk "Stop Writing Classes" for more about this problem.

  2. A new ascii_gsm data structure gets built each time you call IsGsm.is_gsm, but each time the data structure is the same. It would be simpler if this were a global variable. Then it would only have to be constructed once.

  3. The ascii_gsm data structure is poorly named: it is a mapping whose keys are Unicode code points for characters that can be encoded.

  4. The values of the ascii_gsm data structure are always True. This is a waste of space: if you just need the keys, then Python has a set datatype.

  5. The code in the post thinks that FORM FEED cannot be encoded:

    >>> IsGSM.is_gsm('\N{FORM FEED}')
    False
    

    but according to the "Basic Character Set Extension" table in Wikipedia, FORM FEED can be encoded as the two bytes 1B 0A.

  6. I think it would be clearer and less risky to use Python's Unicode named escapes, and write \N{ESCAPE} instead of \x1b. The former is easier to read, easier to check, and Python will raise an error if you make a typo:

    >>> '\N{ESACPE}'
      File "<stdin>", line 1
    SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 0-9: unknown Unicode character name
    

    With hexadecimal escapes, Python won't help you if you write \x1c by mistake for \x1b.

  7. But can ESCAPE really be encoded as 7-bit GSM? It looks to me as though it cannot — Wikipedia says:

    Note that the second part of the table is only accessible if the GSM device supports the 7-bit extension mechanism, using the ESC character prefix. Otherwise, the ESC code itself is interpreted as a space, and the following character will be treated as if there was no leading ESC code.

    So whether the device supports the extension mechanism or not, ESCAPE cannot be encoded.

  8. It's not clear why you want to normalize \xc2. This is LATIN CAPITAL LETTER A WITH CIRCUMFLEX, and when normalized as NFKD this becomes LATIN CAPITAL LETTER A followed by COMBINING CIRCUMFLEX ACCENT. But COMBINING CIRCUMFLEX ACCENT can't be encoded as 7-bit GSM, so the normalization doesn't help. I think this needs explanation, or perhaps just removing.

2. Revised code

I think that it would make more sense here to build a codec. The idea is to write an encode function that encodes Unicode text as 7-bit GSM, using the same interface as Python's own codecs.encode. Then callers will be able to write code like this to detect text that can't be encoded, using the "strict" error handler:

try:
    gsm_encoded_bytes = encode(text, 'strict')
except UnicodeEncodeError:
    # Can't encode text as GSM

This interface also supports more graceful handling of errors, for example if the "replace" error handler is used, then the output will have 3F when a character can't be encoded:

>>> encode('abcω', 'replace')
b'abc?'

Here's a start on an implementation:

import codecs

# Name of the codec.
_NAME = 'gsm03.38'

# GSM 03.38 Basic Character Set
_BASIC_CHARACTERS = (
    "@£$¥èéùìòÇ\nØø\rÅåΔ_ΦΓΛΩΠΨΣΘΞ\N{ESCAPE}ÆæßÉ"
    " !\"#¤%&'()*+,-./0123456789:;<=>?"
    "¡ABCDEFGHIJKLMNOPQRSTUVWXYZÄÖÑܧ"
    "¿abcdefghijklmnopqrstuvwxyzäöñüà")

# GSM 03.38 Basic Character Set Extension. Note that CR2 and SS2 are
# not Unicode characters, so don't appear in this table.
_EXTENSION_CHARACTERS = {
    '\N{FORM FEED}': 0x0A,
    '^': 0x14,
    '{': 0x28,
    '}': 0x29,
    '\\': 0x2F,
    '[': 0x3C,
    '~': 0x3D,
    ']': 0x3E,
    '|': 0x40,
    '€': 0x65,
}

# These are not in the GSM 03.38 specification but we provide them for
# convenience.
_ADDITIONAL_CHARACTERS = {
    '\N{NO-BREAK SPACE}': 0x20,
}

# Build mapping from Unicode character to GSM 03.38 encoding
_CHARMAP = {c: bytes([i]) for i, c in enumerate(_BASIC_CHARACTERS)}
del _CHARMAP['\N{ESCAPE}']
for char, encoding in _EXTENSION_CHARACTERS.items():
    _CHARMAP[char] = bytes([0x1B, encoding])
for char, encoding in _ADDITIONAL_CHARACTERS.items():
    _CHARMAP[char] = bytes([encoding])

def encode(input, errors='strict'):
    error_handler = codecs.lookup_error(errors)
    result = bytearray()
    i = 0
    while i < len(input):
        c = input[i]
        if c in _CHARMAP:
            result.extend(_CHARMAP[c])
            i += 1
        else:
            exc = UnicodeEncodeError(_NAME, input, i, i + 1, "no encoding")
            replace, i = error_handler(exc)
            for c in replace:
                result.extend(_CHARMAP[c])
    return bytes(result)

With some more work we could add a decode function that decodes 7-bit GSM as Unicode, using the same interface as Python's own codecs.decode function. Then we will be able to register our functions with the codec registry, allowing users to write code like:

try:
    gsm_encoded_bytes = text.encode('gsm03.38', 'strict')
except UnicodeEncodeError:
    # Can't encode text as GSM

But in fact there's already a GSM 03.38 codec on the Python Package Index, and it probably makes sense to use that instead of writing your own.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.