Une constante de chaîne doit-elle être définie si elle ne doit être utilisée qu'une seule fois?

24

Nous implémentons un adaptateur pour Jaxen (une bibliothèque XPath pour Java) qui nous permet d'utiliser XPath pour accéder au modèle de données de notre application.

Cela se fait en implémentant des classes qui mappent des chaînes (transmises depuis Jaxen) en éléments de notre modèle de données. Nous estimons que nous aurons besoin d'environ 100 classes avec plus de 1000 comparaisons de chaînes au total.

Je pense que la meilleure façon de le faire est de simples instructions if / else avec les chaînes écrites directement dans le code - plutôt que de définir chaque chaîne comme une constante. Par exemple:

public Object getNode(String name) {
    if ("name".equals(name)) {
        return contact.getFullName();
    } else if ("title".equals(name)) {
        return contact.getTitle();
    } else if ("first_name".equals(name)) {
        return contact.getFirstName();
    } else if ("last_name".equals(name)) {
        return contact.getLastName();
    ...

Cependant, on m'a toujours enseigné que nous ne devons pas incorporer des valeurs de chaîne directement dans le code, mais plutôt créer des constantes de chaîne. Cela ressemblerait à quelque chose comme ceci:

private static final String NAME = "name";
private static final String TITLE = "title";
private static final String FIRST_NAME = "first_name";
private static final String LAST_NAME = "last_name";

public Object getNode(String name) {
    if (NAME.equals(name)) {
        return contact.getFullName();
    } else if (TITLE.equals(name)) {
        return contact.getTitle();
    } else if (FIRST_NAME.equals(name)) {
        return contact.getFirstName();
    } else if (LAST_NAME.equals(name)) {
        return contact.getLastName();
    ...

Dans ce cas, je pense que c'est une mauvaise idée. La constante ne sera utilisée qu'une seule fois, dans la getNode()méthode. L'utilisation directe des chaînes est tout aussi facile à lire et à comprendre que l'utilisation des constantes et nous évite d'écrire au moins mille lignes de code.

Y a-t-il donc une raison de définir des constantes de chaîne pour un usage unique? Ou est-il acceptable d'utiliser directement des chaînes?


PS. Avant que quiconque ne suggère d'utiliser des énumérations à la place, nous avons fait un prototype, mais la conversion d'énumérations est 15 fois plus lente que la simple comparaison de chaînes, donc ce n'est pas envisagé.


Conclusion: Les réponses ci-dessous ont élargi la portée de cette question au-delà des constantes de chaîne, j'ai donc deux conclusions:

  • Il est probablement correct d'utiliser les chaînes directement plutôt que les constantes de chaîne dans ce scénario, mais
  • Il existe des moyens d'éviter d'utiliser des chaînes, ce qui pourrait être mieux.

Je vais donc essayer la technique du wrapper qui évite complètement les cordes. Malheureusement, nous ne pouvons pas utiliser l'instruction de changement de chaîne car nous ne sommes pas encore sur Java 7. En fin de compte, cependant, je pense que la meilleure réponse pour nous est d'essayer chaque technique et d'évaluer ses performances. La réalité est que si une technique est clairement plus rapide, nous la choisirons probablement quelle que soit sa beauté ou son respect des conventions.

gutch
la source
3
Vous ne prévoyez pas de saisir manuellement un 1000 si les instructions le font?
JeffO
1
Je trouve tellement triste combien quelque chose de désagréable ce simple peut être dans certaines langues…
Jon Purdy
5
Java 7 autorise les chaînes comme switchétiquettes. Utilisez un interrupteur au lieu de ifcascades.
Rétablir Monica - M. Schröder
3
la conversion enum est 15 fois plus lente si vous convertissez une chaîne à sa valeur enum! Passez enum directement et comparez avec une autre valeur enum du même type!
Neil
2
Des odeurs comme HashMap peuvent être une solution.
MarioDS

Réponses:

5

Essaye ça. La réflexion initiale est certainement coûteuse, mais si vous allez l'utiliser plusieurs fois, ce que je pense que vous le ferez, c'est certainement une meilleure solution que ce que vous proposez. Je n'aime pas utiliser la réflexion, mais je me retrouve à l'utiliser quand je n'aime pas l'alternative à la réflexion. Je pense que cela sauvera beaucoup de maux de tête à votre équipe, mais vous devez passer le nom de la méthode (en minuscules).

En d'autres termes, plutôt que de passer "nom", vous passeriez "nom complet" car le nom de la méthode get est "getFullName ()".

Map<String, Method> methodMapping = null;

public Object getNode(String name) {
    Map<String, Method> methods = getMethodMapping(contact.getClass());
    return methods.get(name).invoke(contact);
}

public Map<String, Method> getMethodMapping(Class<?> contact) {
    if(methodMapping == null) {
        Map<String, Method> mapping = new HashMap<String, Method>();
        Method[] methods = contact.getDeclaredMethods();
        for(Method method : methods) {
            if(method.getParameterTypes().length() == 0) {
                if(method.getName().startsWith("get")) {
                    mapping.put(method.getName().substring(3).toLower(), method);
                } else if (method.getName().startsWith("is"))) {
                    mapping.put(method.getName().substring(2).toLower(), method);
                }
            }
        }
        methodMapping = mapping;
    }
    return methodMapping;
}

Si vous devez accéder aux données contenues dans les membres de contact, vous pouvez envisager de créer une classe wrapper pour contact qui dispose de toutes les méthodes pour accéder aux informations requises. Cela serait également utile pour garantir que les noms des champs d'accès resteront toujours les mêmes (c'est-à-dire si la classe wrapper a getFullName () et que vous appelez avec fullname, cela fonctionnera toujours même si getFullName () du contact a été renommé - il provoquerait une erreur de compilation avant de vous permettre de le faire).

public class ContactWrapper {
    private Contact contact;

    public ContactWrapper(Contact contact) {
        this.contact = contact;
    }

    public String getFullName() {
        return contact.getFullName();
    }
    ...
}

Cette solution m'a sauvé plusieurs fois, à savoir quand je voulais avoir une seule représentation de données à utiliser dans les tables de données jsf et quand ces données devaient être exportées dans un rapport à l'aide de jasper (qui ne gère pas bien les accesseurs d'objets compliqués dans mon expérience) .

Neil
la source
J'aime l'idée d'un objet wrapper avec les méthodes appelées via .invoke(), car il supprime entièrement les constantes de chaîne. Je ne suis pas si désireux de réfléchir à l'exécution pour configurer la carte, bien que peut-être que l'exécution getMethodMapping()dans un staticbloc soit OK pour que cela se produise au démarrage plutôt qu'une fois le système en cours d'exécution.
gutch
@gutch, le modèle wrapper est celui que j'utilise souvent, car il a tendance à poser de nombreux problèmes liés à l'interface / au contrôleur. L'interface peut toujours utiliser le wrapper et en être satisfait et le contrôleur peut être retourné entre-temps. Tout ce que vous devez savoir, c'est quelles données vous souhaitez mettre à disposition dans l'interface. Et encore une fois, je dis pour souligner, je n'aime pas normalement la réflexion, mais s'il s'agit d'une application Web, c'est tout à fait acceptable si vous le faites au démarrage car le client ne verra aucun de ces temps d'attente.
Neil
@Neil Pourquoi ne pas utiliser BeanUtils d'Apache commons? Il prend également en charge les objets intégrés. Vous pouvez parcourir toute une structure de données obj.attrA.attrB.attrN et il a beaucoup d'autres possibilités :-)
Laiv
Au lieu de mappages avec Maps, je choisirais @Annotations. Quelque chose comme JPA. Pour définir ma propre annotation pour le mappage des entrées de contrôleur (chaîne) avec un attr ou un getter spécifique. Travailler avec Annotation est assez facile et il est disponible à partir de Java 1.6 (je pense)
Laiv
5

Si possible, utilisez Java 7 qui vous permet d'utiliser des chaînes dans les switchinstructions.

Depuis http://docs.oracle.com/javase/tutorial/java/nutsandbolts/switch.html

public class StringSwitchDemo {

    public static int getMonthNumber(String month) {

        int monthNumber = 0;

        if (month == null) {
            return monthNumber;
        }

        switch (month.toLowerCase()) {
            case "january":
                monthNumber = 1;
                break;
            case "february":
                monthNumber = 2;
                break;
            case "march":
                monthNumber = 3;
                break;
            case "april":
                monthNumber = 4;
                break;
            case "may":
                monthNumber = 5;
                break;
            case "june":
                monthNumber = 6;
                break;
            case "july":
                monthNumber = 7;
                break;
            case "august":
                monthNumber = 8;
                break;
            case "september":
                monthNumber = 9;
                break;
            case "october":
                monthNumber = 10;
                break;
            case "november":
                monthNumber = 11;
                break;
            case "december":
                monthNumber = 12;
                break;
            default: 
                monthNumber = 0;
                break;
        }

        return monthNumber;
    }

    public static void main(String[] args) {

        String month = "August";

        int returnedMonthNumber =
            StringSwitchDemo.getMonthNumber(month);

        if (returnedMonthNumber == 0) {
            System.out.println("Invalid month");
        } else {
            System.out.println(returnedMonthNumber);
        }
    }
}

Je n'ai pas mesuré, mais je crois que les instructions switch se compilent dans une table de saut au lieu d'une longue liste de comparaisons. Cela devrait être encore plus rapide.

Concernant votre question réelle: si vous ne l'utilisez qu'une fois, vous n'avez pas besoin d'en faire une constante. Considérez cependant qu'une constante peut être documentée et apparaît dans Javadoc. Cela peut être important pour les valeurs de chaîne non triviales.


la source
2
Concernant la table de saut. Le commutateur String est remplacé par des commutateurs, le premier est basé sur le code de hachage (l'égalité est vérifiée pour toutes les constantes avec le même code de hachage) et sélectionne l'index de branche, les seconds commutateurs sur l'index de branche et sélectionne le code de branche d'origine. Ce dernier est clairement adapté à une table de branche, le premier n'est pas dû à la distribution de la fonction de hachage. Donc, tout avantage de performance est probablement dû à la réalisation basée sur le hachage.
scarfridge
Un très bon point; s'il fonctionne bien, cela pourrait valoir la peine de passer à Java 7 juste pour cela ...
gutch
4

Si vous voulez conserver cela (apporter des modifications non triviales), je pourrais envisager d'utiliser une sorte de génération de code basée sur des annotations (peut-être via CGLib ) ou même simplement un script qui écrit tout le code pour vous. Imaginez le nombre de fautes de frappe et d'erreurs qui pourraient s'introduire dans l'approche que vous envisagez ...

Steven Schlansker
la source
Nous avons envisagé d'annoter les méthodes existantes, mais certains mappages traversent plusieurs objets (par exemple, le mappage "pays" object.getAddress().getCountry()), ce qui est difficile à représenter avec des annotations. Les comparaisons de chaînes if / else ne sont pas jolies, mais elles sont rapides, flexibles, faciles à comprendre et faciles à tester.
gutch
1
Vous avez raison sur le potentiel de fautes de frappe et d'erreurs; ma seule défense est les tests unitaires. Bien sûr, cela signifie encore plus de code ...
gutch
2

J'utiliserais toujours des constantes définies en haut de vos classes. Cela rend votre code plus facile à gérer car il est plus facile de voir ce qui peut être modifié ultérieurement (si nécessaire). Par exemple, "first_name"pourrait devenir "firstName"ultérieurement.

Bernard
la source
Je suis d'accord, cependant, si ce code sera généré automatiquement et que les constantes ne seront pas utilisées ailleurs, alors cela n'a pas d'importance (l'OP dit qu'ils doivent le faire dans 100 classes).
NoChance
5
Je ne vois tout simplement pas l'angle de "maintenabilité" ici, vous changez "first_name" en "givenName" une fois au même endroit dans les deux cas. Cependant, dans le cas des constantes nommées, il vous reste maintenant une variable désordonnée "first_name" qui fait référence à une chaîne "givenName" donc vous voudrez probablement changer cela aussi, donc vous avez maintenant trois changements à deux endroits
James Anderson
1
Avec le bon IDE, ces changements sont triviaux. Ce que je préconise, c'est qu'il est plus évident faire ces changements parce que vous avez pris le temps de déclarer des constantes en haut de la classe et que vous n'avez pas à lire le reste du code de la classe pour apporter ces modifications.
Bernard
Mais lorsque vous lisez l'instruction if, vous devez revenir en arrière et vérifier que la constante contient la chaîne que vous pensez qu'elle contient - rien n'est enregistré ici.
James Anderson
1
Peut-être, mais c'est pourquoi je nomme bien mes constantes.
Bernard
1

Si votre dénomination est cohérente (aka "some_whatever"est toujours associée à getSomeWhatever()), vous pouvez utiliser la réflexion pour déterminer et exécuter la méthode get.

écharpe
la source
Mieux vaut getSome_wwhat (). Peut-être briser l'étui à chameau, mais il est beaucoup plus important de s'assurer que la réflexion fonctionne. De plus, il a l'avantage supplémentaire de vous faire dire: "Pourquoi diable l'avons-nous fait de cette façon ... oh attendez ... Attendez! George ne change pas le nom de cette méthode!"
Neil
0

Je suppose que le traitement des annotations pourrait être la solution, même sans annotations. C'est la chose qui peut générer tout le code ennuyeux pour vous. L'inconvénient est que vous obtiendrez N classes générées pour N classes de modèle. Vous ne pouvez pas non plus ajouter quoi que ce soit à une classe existante, mais écrire quelque chose comme

public Object getNode(String name) {
    return SomeModelClassHelper.getNode(this, name);
}

une fois par classe ne devrait pas être un problème. Alternativement, vous pouvez écrire quelque chose comme

public Object getNode(String name) {
    return getHelper(getClass()).getNode(this, name);
}

dans une superclasse commune.


Vous pouvez utiliser la réflexion au lieu du traitement des annotations pour la génération de code. L'inconvénient est que vous avez besoin de votre code pour compiler avant de pouvoir utiliser la réflexion dessus. Cela signifie que vous ne pouvez pas compter sur le code généré dans vos classes de modèle, sauf si vous générez des stubs.


J'envisagerais également l'utilisation directe de la réflexion. Bien sûr, la réflexion est lente, mais pourquoi est-elle lente? C'est parce qu'il doit faire tout ce que vous devez faire, par exemple, activer le nom du champ.

maaartinus
la source