wikipbx-dev team mailing list archive

[Stas Shtin <antisvin@xxxxxxxxx>]

Hello Dmytro,

I've looked at your code branch and installed it on my development machine.

>From the user's perspective, your work looks very cool. Dialplan editing
will be painless at last. Just a few considerations here:

1. Mochikit effects could be used for changing action element visibility
(see their demos at http://www.mochikit.com/examples/effects/index.html
). This is just a suggestion, as we haven't been heavily using for
visual effect so far.

2. Use "Action #X:" rather than just "Action:" in labels. Using same
label for every action doesn't feel right.

Or maybe do some reordering, so that extension form would look like that:

---------+---------------------
Actions: | + Add new action
---------+---------------------
         | Action FOO  x delete
---------+---------------------
         | Action BAR  x delete
---------+---------------------


Now on to the code.

1. I see that you've added context name to account model. I think you're
only using it for preventing users from hanging up calls not belonging
to him. Ok, but we don't really need to ask user for context name for
this sole purpose. It's better to simply generate context name based on
account ID. I suggest removing that field from the model and use
something like this:

    @property
    def context(self):
        return '__account_%s' % self.id

This way account.context still returns a unique string like it does in
your branch, but we don't ask it from the user.

2. This code:

class ActionSingleForm(forms.Form):
    def __init__(self, account, template, action, *args, **kwargs):
        # ...
        voicetypes =(
('Allison','Allison'),('Mila','Mila'),('William','William'))

        self.voicetypes = voicetypes
        # ...
        self.fields[tv.id].choices = voicetypes

should be written like this:

class ActionSingleForm(forms.Form):
    VOICE_CHOICES = [(voice, voice) for voice in ('Allison', 'Mila',
'William')]

    def __init__(self, account, template, action, *args, **kwargs):
        # ...
        self.fields[tv.id].choices = self.VOICE_CHOICES

The difference is:

* follows DRY principle (Don't Repeat Yourself)
* static data is shared between all class instances, rather than created
each time

Talking about choices, code like:
    SELECTION_CHOICES = ((0, 'Audio file'), (1, 'Extensions'),  (2,
'Gateway'),  (3, 'IVR script'), (4, 'Local endpoint'), (5, 'Voice type') )

can be written like:
    SELECTION_CHOICES = tuple(enumerate('Audio file', 'Extensions',
'Gateway', 'IVR script', 'Local endpoint', 'Voice type'))

Looks much cleaner and less awkward typing.

3. Python style guide (PEP 8) recommends using no more than 79
characters in a line. There are places in older wikipbx code that
violates this rule, but I always try to follow it. Please break long
lines into multiple lines where you have it (models.ActionVariable has
lines with about 140 characters, for instance).

4. Please don't use variable/member names that are two short or joined
without a separator, i.e.:

wikipbx.wikipbxweb.views.edit_extension: formac -> form_action,
wikipbx/wikipbxweb/templates/extensions_edit.html: {% for fm in af.asf
%} -> {% for fm in action_form.single_form %}

"Programs must be written for people to read, and only incidentally for
machines to execute"

5. Don't store strings in class definitions, i.e.:

    labeltxt = "Alias this domain to Dialout SIP profile?"
    help_text = ("When checked, this account's domain will become an " +
                 "alias for the dialout sip profile.")
    aliased = forms.BooleanField(initial=True, required=False,
                                 label=labeltxt,
                                 help_text=help_text)

should be:

    aliased = forms.BooleanField(
        initial=True, required=False,
        label="Alias this domain to Dialout SIP profile?",
        help_text=("When checked, this account's domain will become an "
                   "alias for the dialout sip profile."))

Such reordering doesn't do anything useful, just makes code harder to
follow. There may be left some places where such style is used, but I'll
get rid of it eventually. Also note that there's no need to concatenate
strings with + sign in this case, as python joins strings inside
parentheses anyway.

6. I've noticed you've added sorting to a query in views.soundclips:
.order_by("name")

The proper place to put it is model's option like that:

models.SoundClip(models.Model):
    # ...
    class Meta:
        ordering = ['name']

This sets default ordering for all queries for that model.

7. Rename extensions_edit.html to edit_extension.html . We used to have
more templates like add_foo.html/edit_bar.html, but most of them have
been replaced with a single object_form.html. Still, it's better to
stick to a single convention for naming templates.

As you see, most of this changes are just minor improvements to make
code more readable. Other than that, you did a good job!

Stas.