Skip to content

Refactor code smells for 10, 11, 14 and 15 - #829

Closed
TonyTiago wants to merge 1 commit into
apache:masterfrom
ViKunn:santiago
Closed

TonyTiago wants to merge 1 commit into
apache:masterfrom
ViKunn:santiago

Conversation

@TonyTiago

Copy link
Copy Markdown

Refactorización de code smells: tareas 10, 11, 14 y 15
Se implementaron cuatro refactorizaciones en Apache Commons Compress:

  1. Sustituir algoritmo: se reemplazó el comparador basado en restas por Comparator.comparingInt.
  2. Introducir variable explicativa: se extrajo la condición de comparación en una variable booleana y se utilizó Objects.equals.
  3. Reemplazar condicional anidado por cláusulas de guarda: se extrajo la lógica de selección del códec a un método privado.
  4. Extraer método: se separó la carga del recurso ResourceBundle en el método loadBundle().

Validación: se ejecutó mvn clean test-compile mediante Docker y la compilación finalizó con BUILD SUCCESS. También se ejecutó git diff --check sin errores.

Rama de origen: santiago.

@ecki ecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cn you please make the commit message English and not mention random numbers (I have no idea what they reference), I also have some other points.


// Sort non-predefined attribute bands
final Comparator<NewAttributeBands> comparator = (arg0, arg1) -> arg0.getFlagIndex() - arg1.getFlagIndex();
//Refactorizacion

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don’t comment that code is refactored

}
return result;
}
private static BHSDCodec deriveTokenCodec(final int k, final int l) throws Pack200Exception {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a single use function?

// key.
if (lastArray == array && lastKey == key) {
final boolean sameLookupAsLast = lastArray == array && Objects.equals(lastKey, key);
if (sameLookupAsLast) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don’t that doesnt make it much more readable

"org.apache.commons.compress.harmony.archive.internal.nls.messages"; //$NON-NLS-1$

static {
bundle = loadBundle();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks useful

import java.util.ResourceBundle;

import java.util.logging.Level;
import java.util.logging.Logger;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is JUL used commonly in the project?

@garydgregory

garydgregory commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

-1 change for the sake of change. If you're looking for something to do, please see Jira.

@garydgregory
garydgregory marked this pull request as draft October 29, 2026 11:07
@garydgregory

Copy link
Copy Markdown
Member

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants