Discussion :

[Julien N]

Bonsoir.

J'essaie de faire un truc qui dépasse mes compétences.

Je mets à jour un module interne à ma boite, module servant à définir un type d'objet (un propulseur en l’occurrence). Pour le moment ce module implémente une unique classe. Quelque soit le type de propulseur il est créé par cette classe qui implémente de nombreuses méthodes. La classe est bourrée de conditions car certains propulseurs ont des attributs que d'autres non pas, de même pour les méthodes. Bref c'est le bor**l. Je m'efforce de rationaliser tout ça en créant plusieurs classes, une classe par type de propulseur.

L'idée est la suivante:

• Une base class définissant les attributs et méthodes communes à tout type de propulseur
• Plusieurs classes héritant de la base class avec des paramètres/attributs et méthodes particulieres
• Une factory (fonction) permettant de faire l'interface entre les nouvelles classes et le reste du code plus ancien

Voici le code. J'ai essayé de simplifier au max, sans grand succès:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
# -*- coding: utf-8 -*-
 
class BaseThruster(object):
    def __init__(self, name, value, option=None):
        self.name = name
        self.value = value
        self.option = option
 
    def __str__(self):
        return str(self.name) + ' of type ' + self.__class__.__name__
 
class ThrusterA(BaseThruster):
    def __init__(self, special_arg=None, *args, **kwargs):
        BaseThruster.__init__(self, *args, **kwargs)
        self.special_arg = special_arg
 
class ThrusterB(BaseThruster):
    def __init__(self, special_arg=None, other_arg= None, *args, **kwargs):
        BaseThruster.__init__(self, *args, **kwargs)
        self.special_arg = special_arg
        self.other_arg = other_arg
 
def factory(*args, **kwargs):
    kws = kwargs.copy()
    special_arg = kws.pop('special_arg', None)
    other_arg = kws.pop('other_arg', None)
    if kwargs['value'] == 1:
        return ThrusterA(special_arg, **kws)
    elif kwargs['value'] == 2:
        return ThrusterB(special_arg, other_arg, **kws)
    else:
        raise ArgumentError('Some text')
 
 
if __name__ == '__main__':
 
    obj_params_1 = {'name': '1', 'value': 1, 'special_arg': 'foo'}
    obj_params_2 = {'name': '2', 'value': 2, 'special_arg': 'foo', 'other_arg': 'bar'}
 
    obj_1 = factory(**obj_params_1)
    obj_2 = factory(**obj_params_2)
    # Don't work as expected:
    obj_3 = factory('toto',  1, arg3=None, other_arg='tata')
 
    print obj_1

Le problème c'est que je trouve tout cela bancal. Ceci par exemple:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

def __init__(self, special_arg=None, *args, **kwargs)

Est-ce correct de placer une argument par défaut avant le reste des arguments? Je ne crois pas. Mais je ne parviens pas à faire sans.
Et je ne peux plus créer mes objets sans unpacker depuis un dico:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

obj_3 = factory('toto',  1, arg3=None, other_arg='tata')

Est-ce que vous avez des conseils sur la bonne pratique? Un lien peut-être vers un tuto ou quelque chose expliquant cela?

Merci par avance.

J

[VinsS]

Salut,

Voyons divers cas de figure:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
Python 2.7.3 (default, Dec 18 2014, 19:03:52) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> def factory(*args, **kwargs):
...     print args, kwargs
... 
>>> o1 = {'name': '1', 'value': 1, 'special_arg': 'foo'}
>>> factory(o1)
({'special_arg': 'foo', 'name': '1', 'value': 1},) {}
>>> factory(**o1)
() {'special_arg': 'foo', 'name': '1', 'value': 1}
>>> factory('abc', o1)
('abc', {'special_arg': 'foo', 'name': '1', 'value': 1}) {}
>>> factory('abc', **o1)
('abc',) {'special_arg': 'foo', 'name': '1', 'value': 1}
>>> factory('toto',  1, arg3=None, other_arg='tata')
('toto', 1) {'arg3': None, 'other_arg': 'tata'}
>>> factory('toto',  1, arg3=None, other_arg='tata', **o1)
('toto', 1) {'special_arg': 'foo', 'arg3': None, 'name': '1', 'value': 1, 'other_arg': 'tata'}
>>>

Ça offre beaucoups de possibilités, dans le pire des cas tu dois parser arg pour vérifier s'il contient un dico.

Sinon, en modifiant légèrement ton code:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
def factory(*args, **kwargs):
    kws = kwargs.copy()
    special_arg = kws.pop('special_arg', None)
    other_arg = kws.pop('other_arg', None)
    try:
        val = kwargs['value']
        if val == 1:
            return ThrusterA(special_arg, **kws)
        elif val == 2:
            return ThrusterB(special_arg, other_arg, **kws)
        else:
            raise ArgumentError('Some text')
    except Exception as why:
        print 'factory error: %s' % why
 
if __name__ == '__main__':
    obj_params_1 = {'name': '1', 'value': 1, 'special_arg': 'foo'}
    obj_params_2 = {'name': '2', 'value': 2, 'special_arg': 'foo', 'other_arg': 'bar'}
    obj_1 = factory(**obj_params_1)
    print obj_1, obj_1.value, obj_1.option
    obj_2 = factory(**obj_params_2)
    print obj_2, obj_2.value, obj_2.option
    obj_3 = factory('toto',  1, arg3=None, other_arg='tata')

J'obtiens ceci:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
 
1 of type ThrusterA 1 None
2 of type ThrusterB 2 None
factory error: 'value'

Ce qui me semble correct. Non ?

[Julien N]

Salut,

Au fait j'aurais préféré pouvoir créer mes objets des deux façons:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
obj_1 = factory(**obj_params_1)
obj_3 = factory('toto',  1, arg3=None, other_arg='tata')

Ce besoin vient de certains modules et programmes dérivés qui définissent ces objets indifféremment. En poussant un peu je parviens à ceci:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
def factory(*args, **kwargs):
    """Factory function"""
    if 'value' in kwargs:
        typ = kwargs['value']
    elif len(args) == 2:
        typ = args[-1]
    else:
        raise ArgumentError('Wrong definition')
    kws = kwargs.copy()
    special_arg = kws.pop('special_arg', None)
    other_arg = kws.pop('other_arg', None)
    try:
        if typ == 1:
            return ThrusterA(special_arg, *args, **kws)
        elif typ == 2:
            return ThrusterB(special_arg, other_arg, *args, **kws)
    except Exception as e:
        print 'factory error: %s' % e
 
 
if __name__ == '__main__':
 
    obj_params_1 = {'name': '1', 'value': 1, 'special_arg': 'foo'}
    obj_params_2 = {'name': '2', 'value': 2, 'special_arg': 'foo', 'other_arg': 'bar'}
 
    obj_1 = factory(**obj_params_1)
    obj_2 = factory(**obj_params_2)
    obj_3 = factory('toto',  1, option='test', other_arg='tata')

ça fonctionne. Par contre, si les paramètres de la base class venaient à varier, il faudrait revoir également le contenu de la factory. Y'a peut-être mieux. D'ailleurs je passe peut-être à côté d'un gros problème de conception. Si c'est le cas n'hésitez pas à me le signaler.

J

[fred1599]

C'est le bordel parce-que tu as des arguments non nommés, ce qui rend le code difficile à lire, mais surtout à maintenir...

On peut rentrer plein d'arguments et il est impossible de dire si ces arguments sont corrects ou non. Par contre en le nommant, on impose une utilisation précise lors de l'initialisation de ta classe et on limite les risques d'erreurs.

• Que représente ces arguments non nommés ?
• Pourquoi ne pas mettre juste des arguments nommés ?
• Pourquoi construire en fonction des arguments, plutôt que par le nom du propulseur ?

Bref des questions auxquelles je ne peux répondre à ta place...

[Julien N]

Salut.

Cest sûr, c'est un peu le bordel. L'Idee principale derriere cette factory c'est de pas se soucier du code deja existant. Je rajoute des types et a l'avenir de nouveaux types seront probablement crees. Le code est plus simple a maintenir si la creation de l'objet reste inchangé.

Ce que je ne dis pas c'est que les objets en questions sont crees apres avoir parsé un fichier d'inputs où le type est declaré par un caractere. Il me semble simple alors d'utiliser ce parametre comme condition a la creation meme de l'objet.

Apres libre a moi de ne pas m'engager dans cette voie, et de plutôt remplacer la creation des thrusters de sorte a nommer tous Les arguments explicitement. Cest un peu de boulot mais gerable.

ju

[fred1599]

Difficile de t'aider plus, il faudrait soit plus de détails, soit plus généralisé ton cas, ici on est entre les deux cas, ce qui rend assez flou l'idée que tu peux avoir de ta solution...

Expérimenté comme tu es en python, tu dois sans doute te rendre compte de ce qui pourrait être le meilleur choix, et à la limite, on peut parler de langage objet si le besoin s'en fait ressentir, mais attention, ce n'est pas toujours ou forcément une obligation de l'utiliser.

Personnellement, je pense que dans cette situation, prévoir dans une fonction un nombre d'arguments indéfinis est dangereux... Il faudrait prendre le pire des cas, situation existante rendant difficile l'exécution de cette fonction. Si c'est difficile à gérer, peut-être faut-il modifier ta vision pour résoudre ce problème.

[Julien N]

Salut,

Personnellement, je pense que dans cette situation, prévoir dans une fonction un nombre d'arguments indéfinis est dangereux...

Je pense voir le problème. Je suis loin d'être expérimenté. Au contraire, je suis ravi de pouvoir m'améliorer. Je pense partir sur cette modification ci où je nomme explicitement les arguments nécessaires, seuls les options restent sous la forme d'un dictionnaire:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
def factory(name, typ, **kwargs):
    """Factory function
    
    name : str
        Name of the object
    typ : int
        Type of the object. Must be 1 or 2.
    option : str, optional
        Option of the objct
    special_arg : ...
    other_arg : ...
    ...
    
    """
    kws = kwargs.copy()
    args = (name, typ)
    special_arg = kws.pop('special_arg', None)
    other_arg = kws.pop('other_arg', None)
    try:
        if typ == 1:
            return ThrusterA(special_arg, *args, **kws)
        elif typ == 2:
            return ThrusterB(special_arg, other_arg, *args, **kws)
        else:
            raise ValueError('Wrong object type %i. Must be 1 or 2' % (typ,))
    except Exception as e:
        print 'factory error: %s' % e

Cette forme me semble suffisament robuste pour répondre à mes besoins.

Ju

[fred1599]

Et pourquoi pas juste typ comme argument ?

[Julien N]

Car je reprends les meme arguments que la baseclass.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
class BaseThruster(object):
    def __init__(self, name, typ, option=None):

[fred1599]

Ah ok, je croyais que typ et name avait une relation... et qu'avec l'un on pouvait déduire l'autre

[wiztricks]

Salut,

Je pense partir sur cette modification ci où je nomme explicitement les arguments nécessaires

Poussez le raisonnement à:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
def factory(name, value, **kw):
    if value == 1:
        return ThrusterA(name, **kw)
    elif value == 2:
        return ThrusterB(name, **kw)
    else:
        raise ArgumentError('Some text')

value est le deuxième argument passé ou pas sous la forme value=n.
C'est sa valeur qui fait le dispatch entre A et B.
Dit autrement, value est un attribut qui semble dépendre de la classe assignée:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
class ThrusterA(BaseThruster):
    value = 1
    def __init__(self, name, option=None, special_arg=None, **kw):
        BaseThruster.__init__(self, name, option)
        self.special_arg = special_arg
 
class ThrusterB(BaseThruster):
    value = 2
    def __init__(self, name, option=None, special_arg=None, other_arg= None, **kw):
        BaseThruster.__init__(self,  name, option)
        self.special_arg = special_arg
        self.other_arg = other_arg

Mais ce n'est qu'une idée.

- W