Scout toujours !

le 08/06/2012 par Mathieu Gandin
Tags: Product & Design

"cleancode"

Dans son livre « Clean Code », Robert C. Martin nous présente un principe essentiel pour écrire du beau code, la fameuse règle des boy-scouts (« The Boy Scout Rule ») qui se résume en une phrase : « Toujours laisser un endroit dans un état meilleur que celui où vous l’avez trouvé ».

Si nous livrons tous du code dans un état plus propre que  celui où nous l’avons trouvé, alors le code ne risque pas de se détériorer. Le nettoyage de code n’a pas besoin d’être immense, il suffit souvent de changer un nom de variable, de découper une méthode trop longue, d’éliminer de la duplication de code, de nettoyer une instruction qui comprend trop de if, pour que le code soit meilleur. Si l’amélioration continue fait partie intégrante du quotidien de l’IT alors « The Boy Scout Rule », telle que décrite par Robert C. Martin, en est son application la plus importante sur l’écriture de code.

Comment pouvons-nous faire pour que le code s’améliore en continu dans le temps ? Voici une mise en situation nous permettant d’illustrer ce principe sur une petite portion de code :

La scène se passe à la fin du stand-up meeting, sur le plateau d’une équipe de développement, il doit être 9h52 …

-       J’ai trouvé le problème du bug X342, il manque le mapping du nom du contact tiers, je corrige ça en 5 minutes, et je livre … -       Ah oui je me souviens de ce code livré à l’arrache le dernier jour d’une itération … Je te rappelle que nous avons défini comme règle d’équipe de toujours remanier le code pour le rendre plus propre qu’avant. Peux-tu prendre le temps de correctement le tester et refactorer ? -       Bien sûr, et tu veux bien binômer avec moi alors ? -       OK ! - Voici la portion de code où le bug semble provenir :

public final static List findByNoTiersIdTypeUsage(Context context, String noIdentiteTiers, long idUsageAdrTiers) throws com.refTiers.tech.exception.RuntimeException {
	List lst = new ArrayList();
	PreparedStatement pStmt = null;
	ResultSet rs = null;
	Connection connexion = context.getConnection();
	try {
		pStmt = connexion.prepareStatement( " SELECT * FROM "+TAB_ADRESSE_TIERS+" WHERE "+FIELD_NO_IDENTITE_TIERS
				+" = ? AND "+FIELD_ID_USAGE_ADR_TIERS+" = ? ");
		pStmt.setString(1, noIdentiteTiers);
		pStmt.setDouble(2, idUsageAdrTiers);
		rs = pStmt.executeQuery();
		while (rs.next())
			lst.add(convert(rs));
	} catch (SQLException sqe) {
		throw new RuntimeException(sqe);
	} finally {
		DBUtil.closeRsStmt(rs, pStmt);
	}
	return lst;
}

-       Bon, il y a du boulot !  Première étape, écrivons un test, tu me laisses prendre le clavier ? -       Tiens ! -       Voici ce que je te propose, qu’en penses-tu ?

@Test
public void shouldFindAddressByNoIdTypeUsage() throws RuntimeException {
	Context context = new Context();
	List result = AdresseTiersDB.findByNoTiersIdTypeUsage(context, "123", 1L);
	Assert.assertNotNull(result);
}

- A mon avis, ça ne marchera pas, l’objet context me semble contenir un état qui doit faire fonctionner correctement le code, et nous ne l’avons pas initialisé. - Lançons le test pour voir ! - Kaboom ! Le test ne passe pas, une erreur d’accès à la base de données, il faut « mocker » l’objet context et sûrement la classe java.sql.Connection … - Pour simuler l’environnement de base de données, il nous faut un framework de mock. Tu en connais un qui ferait l’affaire ? - Oui, Mockito. Peux tu me donner le clavier que je l’ajoute à la gestion de configuration du projet en mettant à jour le fichier pom.xml ? - Tiens ! - Voilà, on peut remanier le code avec des mocks.

org.mockito
  	mockito-all
  	1.9.0
  	test

- Et maintenant, comment comptes-tu remanier le code ? - Tout d’abord j’extrais la création de l’objet context dans une méthode initializeContext(). Ca te va ?

@Test
public void shouldFindAddressByNoIdTypeUsage() throws RuntimeException {
	Context context = initializeContext();
	List result = AdresseTiersDB.findByNoTiersIdTypeUsage(context, "123", 1L);
	Assert.assertNotNull(result);
	Mockito.verify(connection);
}

- Ca me va ! - Ensuite je complète la méthode initializeContext() avec le code de simulation de la base de données :

private Context initializeContext () {
	return new Context() {

		@Override
		public Connection getConnection() {
			connection = Mockito.mock(Connection.class);
			preparedStatement =  Mockito.mock(PreparedStatement.class);
			resultSet = Mockito.mock(ResultSet.class);
	
		     try {
                     Mockito.
                     when(connection.prepareStatement(Mockito.
                        anyString())).
                     thenReturn(preparedStatement);

		          Mockito.
                     when(preparedStatement.executeQuery()).
                     thenReturn(resultSet);
		     } catch (SQLException e) {
			     e.printStackTrace();
		     }
		     return connection;
		}
			
	};
}

- Ok, maintenant on relance le test ! La barre passe au vert ! - Avant d’écrire un test supplémentaire pour faire apparaître le bug, je souhaite modifier la signature de la méthode en enlevant les static, le composant sera plus modulaire ainsi. - Très bonne idée, comme ça le code sera plus propre qu’avant, commençons par modifier le test :

@Test
public void shouldFindAddressByNoIdTypeUsage() throws RuntimeException {
	Context context = initializeContext();
	AdresseTiersDB adresseTiersDB = new AdresseTiersDB();
	List result = adresseTiersDB.findByNoTiersIdTypeUsage(context, "123", 1L);
	Assert.assertNotNull(result);
	Mockito.verify(connection);
}

- Bien, et maintenant, on modifie le code

public List findByNoTiersIdTypeUsage(Context context, String noIdentiteTiers, long idUsageAdrTiers) throws com.refTiers.tech.exception.RuntimeException {
…

- Toujours pour rendre le code plus propre, notamment en introduisant des principes d’injection de dépendance, je te propose de déplacer l’objet context, qui est passé en paramètre de la méthode findByNoTiersIdTypeUsage, en paramètre du constructeur. Voilà ce que ça donne, qu’en penses-tu ?

private Context context;
	
public AdresseTiersDB(Context context) {
	this.context = context;
}

public List findByNoTiersIdTypeUsage(String noIdentiteTiers, long idUsageAdrTiers) throws com.refTiers.tech.exception.RuntimeException {
	List lst = new ArrayList();
	PreparedStatement pStmt = null;
	ResultSet rs = null;
	Connection connexion = context.getConnection();
	try {
		pStmt = connexion.prepareStatement( " SELECT * FROM "+TAB_ADRESSE_TIERS+" WHERE "+FIELD_NO_IDENTITE_TIERS
					+" = ? AND "+FIELD_ID_USAGE_ADR_TIERS+" = ? ");
		pStmt.setString(1, noIdentiteTiers);
		pStmt.setDouble(2, idUsageAdrTiers);
		rs = pStmt.executeQuery();
		while (rs.next())
			lst.add(convert(rs));
	} catch (SQLException sqe) {
		throw new RuntimeException(sqe);
	} finally {
		DBUtil.closeRsStmt(rs, pStmt);
	}
	return lst;
}

- C’est bien, le test est toujours vert, mais je pense qu’on peut aller plus loin, peux-tu me passer le clavier ? - Tiens ! - Ok, voilà ce que je te propose de renommer pour que ce soit plus clair :

public List findByNumberIdentityOfTiersAndByIdentifierUsageAdresseTiers(String numberIdentityOfTiers, long identifierUsageAdresseTiers) throws com.refTiers.tech.exception.RuntimeException {
	List listOfAdresseTiersBO = new ArrayList();
	PreparedStatement preparedStatement = null;
	ResultSet resultSet = null;
	Connection connection = context.getConnection();
	try {
		preparedStatement = connection.prepareStatement( " SELECT * FROM "+TAB_ADRESSE_TIERS+" WHERE "+FIELD_NO_IDENTITE_TIERS
					+" = ? AND "+FIELD_ID_USAGE_ADR_TIERS+" = ? ");
		preparedStatement.setString(1, numberIdentityOfTiers);
		preparedStatement.setDouble(2, identifierUsageAdresseTiers);
		resultSet = preparedStatement.executeQuery();
		while (resultSet.next())
			listOfAdresseTiersBO.add(convert(resultSet));
	} catch (SQLException sqlException) {
		throw new RuntimeException(sqlException);
	} finally {
		DBUtil.closeRsStmt(resultSet, preparedStatement);
	}
	return listOfAdresseTiersBO;
}

- C’est mieux, le nommage des variables est plus clair que les abréviations qui se trouvaient avant. Pour le prochain remaniement de code, je te propose de corriger un autre problème : nous avons des deux langages dans cette classe, du Java et du SQL. - Exact ! Je sors le code SQL dans une méthode dédiée :

private String constructSqlRequest() {
	return " SELECT * FROM "+TAB_ADRESSE_TIERS+" WHERE "+FIELD_NO_IDENTITE_TIERS
				+" = ? AND "+FIELD_ID_USAGE_ADR_TIERS+" = ? ";
}

- Parfait ! Ensuite nous pouvons typer la liste retournée en utilisant un Generics, d’abord dans mon test :

@Test
public void shouldFindAddressByNoIdTypeUsage() throws RuntimeException {
	Context context = getContext();
	AdresseTiersDB adresseTiersDB = new AdresseTiersDB(context);
	List result = adresseTiersDB.findByNumberIdentityOfTiersAndByIdentifierUsageAdresseTiers("123", 1L);
	Assert.assertNotNull(result);	
	Mockito.verify(connection);
}

- Ok, bon réflexe de changer d’abord le test, qui définit le contrat de ta classe, avant de modifier son implémentation - Bien, et voici le code, qui est plus propre :

public List findByNumberIdentityOfTiersAndByIdentifierUsageAdresseTiers(String numberIdentityOfTiers, long identifierUsageAdresseTiers) throws com.refTiers.tech.exception.RuntimeException {
		
     List listOfAdresseTiersBO = new ArrayList();
	
PreparedStatement preparedStatement = null;
	ResultSet resultSet = null;
	Connection connection = context.getConnection();
	try {
		preparedStatement = connection.prepareStatement( constructSqlRequest());
		preparedStatement.setString(1, numberIdentityOfTiers);
		preparedStatement.setDouble(2, identifierUsageAdresseTiers);
		resultSet = preparedStatement.executeQuery();
		while (resultSet.next())
			listOfAdresseTiersBO.add(convert(resultSet));
	} catch (SQLException sqe) {
		throw new RuntimeException(sqe);
	} finally {
		DBUtil.closeRsStmt(resultSet, preparedStatement);
	}
	return listOfAdresseTiersBO;
}

- Maintenant le code est dans un état bien plus propre que celui dans lequel nous l’avons trouvé. Il semblerait que le problème soit dans la fonction convert() qui ne mappe pas le nom du contact tiers. - Je te propose de faire apparaître ce problème en enrichissant notre test avec des données. - Ok, il nous suffit de définir un cas de test où l’on récupère depuis la base de données un seul objet de type AdresseTiersBO, puis nous allons vérifier si le nom du contact tiers est bien présent. - Le test doit être en erreur pour faire apparaître le bug.

@Test
public void shouldFindAddressByNoIdTypeUsage() throws RuntimeException {
	Context context = initializeContext();
	AdresseTiersDB adresseTiersDB = new AdresseTiersDB(context);
	List result = adresseTiersDB.findByNumberIdentityOfTiersAndByIdentifierUsageAdresseTiers("123", 1L);
	Assert.assertNotNull(result);	
	AdresseTiersBO adresseTiersBO = result.get(0);
	Assert.assertEquals("Bruce Wayne", adresseTiersBO.getNomContactTiers());
	Mockito.verify(connection);
}

- D’accord, mais le test ne fonctionnera pas, car le jeu de données est vide. Peux-tu me passer le clavier pour enrichir notre code de bouchonnage en conséquence ? - Tiens ! - Ok, voilà le code !

connection = Mockito.mock(Connection.class);
preparedStatement =  Mockito.mock(PreparedStatement.class);
resultSet = Mockito.mock(ResultSet.class);
try {
					Mockito.when(connection.prepareStatement(Mockito.anyString())).thenReturn(preparedStatement);
					Mockito.when(preparedStatement.executeQuery()).thenReturn(resultSet);
					Mockito.when(resultSet.next()).thenReturn(true).thenReturn(false);					
					Mockito.when(resultSet.getString(AdresseTiersDB.FIELD_NOM_CONTACT_TIERS)).thenReturn("Bruce Wayne");
} catch (SQLException e) { … }

- Bien, relançons le test, il est en rouge ! - Ca y est, nous avons capturé notre bug, maintenant, il est temps de le corriger, dans la fonction convert() il manquait le mapping entre l’instance de ResultSet et l’objet de type AdresseTiersBO. - Très bien, corrigeons et relançons le test - Il est vert ! - Bravo ! - Terminons notre binômage par un débriefing, qu’avons nous appris ? - Nous avons nettoyé le code, maintenant il est testé, remanié et relativement propre - Et puis ? - Maintenant nous avons des tests de non-régression automatique sur ce code, et je vois encore quelques pistes d’amélioration : complétement sortir le code SQL, peut être en introduisant un framework comme Spring-DAO, ensuite il reste encore quelques Magic Numbers à placer dans des constantes et … - Et encore ? - Bien sûr, le bug est corrigé et j’ai l’impression que le code de notre application et notre environnement de travail se sont améliorés en une heure de binômage !...

Pour finir :

Tu es un tech-lead et tu souhaites que la Boy-Scout Rule soit appliquée dans ton équipe ? Quelques conseils :

  • Commence par poser un atelier avec les développeurs qui codent avec toi pour définir les règles partagées et travaille avec ton équipe sur comment installer la Boy-Scout Rule ?
  • Binômage et Coding Dojo seront tes alliés pour diffuser les pratiques de tests et de refactoring te permettant d’ancrer la Boy-Scout Rule dans ton équipe.
  • Quand tu recrutes un développeur, code une demi-heure avec lui pour sentir ses appétences avec la Boy Scout Rule.