clean code

&

code smell

clean code

Qu'est-ce qu'un code propre ?

ça dépend ...

clean code

• lisibilité
• maintenabilité
• complexité
  • nombres de branches
  • boucles imbriqués
• taille des classes  / méthodes / lignes
• tests

clean code

• complexité cyclomatique < 10
  • boucles imbriqués : pas plus de deux niveaux
• taille des classes  / méthodes / lignes
  • classes : 1000
  • méthode : 100 (tient sur un écran)
  • ligne : 180 (tient sur un écran)
  • lambda : 20
• test : coverage à 50% min => 60% en cible
  • Privilégier les tests de haut niveau
  • Mock, BDD embarqué, Docker : pérennité des tests

clean code

• Single Responsibility Principle : une classe ne doit avoir qu’une seule raison de changer
• Separation Of Concern
• KISS (Keep It Simple Stupid)
• YAGNI (You Aren’t Going to Need It) :  ne pas coder des trucs pour le futur
• DRY : Don't Repeat Yourself
• Ne pas réinventer la roue
• ...

code smell

Hum, ce code il sent mauvais ...

code smell

"Code smell, also known as bad smell, in computer programming code, refers to any symptom in the source code of a program that possibly indicates a deeper problem. According to Martin Fowler, "a code smell is a surface indication that usually corresponds to a deeper problem in the system"

Wikipedia

code smell

• Too Many Null Checks
• Deeply Nested Code
• Method Parameter Mutation
• Multi Responsibility Method
• Too Many if/else
• Too Many Problems ...

code smell

Le temps du refacto

Exemples & Jeux

public String validate(final OrderPivot input) {
        StringBuilder outputErrorBuilder = new StringBuilder();
        // validation du root
        if (StringUtils.isBlank(input.getOrder())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "Order").toString());
        }

        if (StringUtils.isBlank(input.getLastEvent())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "LastEvent").toString());
        }

        if (input.getLastEventDate() == null) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "LastEventDate").toString());
        }

        if (StringUtils.isBlank(input.getOrderType())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "OrderType").toString());
        }

        if (StringUtils.isBlank(input.getSortingSiteId())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "SortingSiteId").toString());
        }

        if (StringUtils.isBlank(input.getTurnoverStoreId())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "TurnoverStoreId").toString());
        }

        if (StringUtils.isBlank(input.getLoyaltyCard())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "LoyaltyCard").toString());
        }

        // validation du Shipment
        if (isValidateShipment(input.getShipment())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "Shipment").toString());
        }

        // validation des Lines
        //...next on the next page

    }

    if(input.getLines().isEmpty() ) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "Lines").toString());
        }else {
            List<SkuLine> lineInError = input.getLines().stream().filter(TranfertRequestValidator::isValidateSkuLine)
                    .collect(Collectors.toList());

            if ( !lineInError.isEmpty()) {
                outputErrorBuilder.append("[ Lines fields must be not empty for lineNumber ");

                // doit etre fais apres si un des lineNumber est null
                List<Integer> lineNumberInError = lineInError.stream().map(SkuLine::getLineNumber)
                        .collect(Collectors.toList());
                for (Integer nb : lineNumberInError) {
                    outputErrorBuilder.append('{').append(nb).append('}');
                }
                outputErrorBuilder.append(']');
            }
        }

        // validation du shipmentManifest
        if(validateShipmentManifest(input.getShipmentManifest())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "ShipmentManifest").toString());
        }

        String outPutError = outputErrorBuilder.toString();
        if(!outPutError.isEmpty()) {
            return outPutError;
        }else {
            return null;
        }

Redondant Stream usage + for loop !

    public String validate(final OrderPivot input) {
        StringBuilder outputErrorBuilder = new StringBuilder();
        // validation du root
        validateStringField(input.getOrder(), outputErrorBuilder, "Order");

        validateStringField(input.getLastEvent(), outputErrorBuilder, "LastEvent");

        validateNotNullField(input.getLastEventDate(), outputErrorBuilder, "LastEventDate");

        validateStringField(input.getOrderType(), outputErrorBuilder, "OrderType");

        validateStringField(input.getSortingSiteId(), outputErrorBuilder, "SortingSiteId");

        validateStringField(input.getTurnoverStoreId(), outputErrorBuilder, "TurnoverStoreId");

        validateStringField(input.getLoyaltyCard(), outputErrorBuilder, "LoyaltyCard");

        // validation du Shipment
        // on next page
    }

    private void validateNotNullField(final Object input, final StringBuilder outputErrorBuilder,
            final String fieldName) {
        if (input == null) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", fieldName).toString());
        }
    }

    private void validateStringField(final String input, final StringBuilder outputErrorBuilder,
            final String fieldName) {
        if (StringUtils.isBlank(input)) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", fieldName).toString());
        }
    }

        // validation des Lines
        if (input.getLines().isEmpty()) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "Lines").toString());
        } else {

            String lineInError = input.getLines().stream().filter(TranfertRequestValidator::isValidateSkuLine)
                    .map(SkuLine::getLineNumber).map(ln -> ln == null ? LINE_NUMBER_EMPTY : ln.toString())
                    .collect(Collectors.joining(DELIMITER_DOUBLE, DELIMITER_PREFIX, DELIMITER_SUFFIX));

            if (!DELIMITER_SUF_PRE.equals(lineInError)) {
                outputErrorBuilder.append("[ Lines fields must be not empty for lineNumber ");
                outputErrorBuilder.append(lineInError);
                outputErrorBuilder.append(']');
            }
        }

        // validation du shipmentManifest
        if (validateShipmentManifest(input.getShipmentManifest())) {
            outputErrorBuilder.append(new KiabiMessage("error.field.is.empty", "ShipmentManifest").toString());
        }

        String outPutError = outputErrorBuilder.toString();
        if (!outPutError.isEmpty()) {
            return outPutError;
        } else {
            return null;
        }

private Boolean validateShipmentManifest(final ShipmentManifest inputShipmentManifest) {
        if(inputShipmentManifest == null) {
            return Boolean.TRUE;
        }
        if (inputShipmentManifest.getShipmentNumber()== null) {
            return Boolean.TRUE;
        }
        if (StringUtils.isBlank(inputShipmentManifest.getHubCode())) {
            return Boolean.TRUE;
        }
        if (StringUtils.isBlank(inputShipmentManifest.getParcelNumber())) {
            return Boolean.TRUE;
        }
        if (StringUtils.isBlank(inputShipmentManifest.getParcelNumberExtended())) {
            return Boolean.TRUE;
        }
        if (StringUtils.isBlank(inputShipmentManifest.getInternalParcelNumber())) {
            return Boolean.TRUE;
        }
        if (inputShipmentManifest.getWeight() == null) {
            return Boolean.TRUE;
        }
        if (StringUtils.isBlank(inputShipmentManifest.getHubParcelNumber())) {
            return Boolean.TRUE;
        }
        if (StringUtils.isBlank(inputShipmentManifest.getCartonType())) {
            return Boolean.TRUE;
        }
        if (inputShipmentManifest.getTransfertNumber() == null) {
            return Boolean.TRUE;
        }
        if (inputShipmentManifest.getShipmentDate() == null) {
            return Boolean.TRUE;
        }
        if (inputShipmentManifest.getPackingDate() ==null) {
            return Boolean.TRUE;
        }
        return Boolean.FALSE;
    }

private Boolean validateShipmentManifest(final ShipmentManifest inputShipmentManifest) {
        return inputShipmentManifest == null || inputShipmentManifest.getShipmentNumber() == null
                || StringUtils.isBlank(inputShipmentManifest.getHubCode())
                || StringUtils.isBlank(inputShipmentManifest.getParcelNumber())
                || StringUtils.isBlank(inputShipmentManifest.getParcelNumberExtended())
                || StringUtils.isBlank(inputShipmentManifest.getInternalParcelNumber())
                || inputShipmentManifest.getWeight() == null
                || StringUtils.isBlank(inputShipmentManifest.getHubParcelNumber())
                || StringUtils.isBlank(inputShipmentManifest.getCartonType())
                || inputShipmentManifest.getTransfertNumber() == null || inputShipmentManifest.getShipmentDate() == null
                || inputShipmentManifest.getPackingDate() == null;
    }

public void format(final LogEvent event, final StringBuilder toAppendTo) {
	final Message msg = event.getMessage();

	//KIA : START Kiabi customization
	Throwable throwable = event.getThrown();
	String exceptionCode = KiabiException.UNKNOWN_EXCEPTION_CODE;
	if(throwable != null){
		if(throwable instanceof KiabiException){
			//if it's a KiabiException : we have an exception cause
			exceptionCode = ((KiabiException) throwable).getExceptionCode();
		}
		else {
			//else, we check on known exception types on the exception itself or it's causes
			Throwable toCheck = throwable;
			while(toCheck != null && exceptionCode.equals(KiabiException.UNKNOWN_EXCEPTION_CODE)){
				if(toCheck instanceof SQLException){
					exceptionCode = ExceptionCodeExtractor.fromSqlException((SQLException) toCheck);
				}
				else if(toCheck instanceof NamingException){
					exceptionCode = ExceptionCodeExtractor.fromNamingException((NamingException) toCheck);
				}
				else if(toCheck instanceof KiabiException){
					//sometimes a KiabiException can be the cause of an other exception, so we need to check this also
					exceptionCode = ((KiabiException) throwable).getExceptionCode();
				}
	
				//check on the cause
				toCheck = toCheck.getCause();
			}
		}
		//TODO make clever analyse on exception to add exception code whenever it's possible
		toAppendTo.append(exceptionCode).append(" - ");
	}

        if (msg instanceof StringBuilderFormattable) {
		final int offset = toAppendTo.length();
		((StringBuilderFormattable) msg).formatTo(toAppendTo);
              //... format of the message
}

 public void format(final LogEvent event, final StringBuilder toAppendTo) {
        final Message msg = event.getMessage();

        //KIA : START Kiabi customization
        if(event.getThrown() != null) {
            //add exception code
            String exceptionCode = ExceptionCodeExtractor.extractExceptionCode(event.getThrown());
            toAppendTo.append(exceptionCode).append(" - ");
        }
        //KIA : STOP Kiabi customization


        //WARNING : bellow this line, this is taken from org.apache.logging.log4j.core.pattern.MessagePatternConverter
        //be carefull that upgrading log4j2 could lead to modification needed on this code and that needs to be manually done!
        if (msg instanceof StringBuilderFormattable) {
            final int offset = toAppendTo.length();
            ((StringBuilderFormattable) msg).formatTo(toAppendTo);
            //... format the messaye
}


public static String extractExceptionCode(Throwable throwable) {
        //if the exception null (or if we go to all the causes and didn't find a match) : return UNKNOWN_EXCEPTION_CODE
        if(throwable == null) {
            return KiabiException.UNKNOWN_EXCEPTION_CODE;
        }

        //check on known exception types to extract a code
        if(throwable instanceof KiabiException){
            return ((KiabiException) throwable).getExceptionCode();
        }
        else if(throwable instanceof SQLException){
            return fromSqlException((SQLException) throwable);
        }
        else if(throwable instanceof NamingException){
            return fromNamingException((NamingException) throwable);
        }

        //recursive check on the cause
        return extractExceptionCode(throwable.getCause());
    }

Live Refactoring Dojo Contest ?