Code Monkey home page Code Monkey logo

scandisk-2.0's Introduction

scandisk-2.0's People

Contributors

manzdev avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar

Forkers

aliushn

scandisk-2.0's Issues

Review

¡Hola Manz!

En general está muy bien, se lee y se entiende de forma clara y concisa, las clases tienen pinta que están separadas por responsabilidades, los nombres de métodos y variables es representativo y los algoritmos usados has hecho que queden simples, cosa que normalmente es lo más complicado hacer. Las cosas que he visto son pequeñas mejoras que bajo mi opinión harán tu código sea incluso más legible, entendible y mantenible.

Esta lógica de obtener un número aleatorio está duplicada tanto aquí como aquí. Hay bastante gestión de números aleatorios, yo intentaría llevármela a una clase. Además, si lo haces así va a ser más fácil testear los componentes ya que podrías mockear esa clase tuya.

Para el tema del aleatorio intentaría que esa clase recibiese por constructor un objeto que te dé un valor aleatorio (por ejemplo el Math.random). Haría esto para que los tests de esta clase se pudiesen hacer fácilmente.

export interface RandomProvider {
  provide(): number
}

export class Random {
  constructor(randomProvider) {
    this.randomProvider = randomProvider
  }

  get(max): number {
    return Math.floor(this.randomProvider.provide() * (max + 1))
  }
}

// Uso real
new Random({ provide: () => Math.random() }).get(10)

// En el test
new Random({ provide: () => 0.5 }).get(10)

Intentaría que todas las dependencias viniesen inyectadas. ¿Cómo haría eso? Pues lo más fácil sería pasarlas por properties. Es un poco pesado pero eso hará que el testeo de tus componentes sea más fácil, ya que en los test le puedes pasar mocks. Esto que estamos haciendo al igual que lo que hemos hecho con las clase Random es Inversión de dependencias, que es uno de los principios SOLID.

Evitar comentarios si puedes explicar lo mismo con código. Por ejemplo aquí si de verdad quisieses incluir ese comentario (que dudo que en este caso sea necesario) lo haría de esta forma:

const createdElement = new ScandiskBar();
this.scandiskBar = createdElement;

Al igual que en estas declaraciones donde hay comentarios yo lo que haría sería añadir más contexto al nombre para que el comentario no sea necesario:

// Scandisk Surface Visual Blocks
const NUM_BLOCKS = 518;

// Calculate clusters blocks-based
const CLUSTERS = ~~(Math.random() * 100000) + 500000;
const CLUSTERS_PER_BLOCK = ~~(CLUSTERS / NUM_BLOCKS);

Teniendo clases podrías moverlas dentro del componente:

export default class ScandiskSurface extends LitElement {
  static SURFACE_VISUAL_BLOCKS = 518

  static getBlockBasedClusters() {
    const clusters = ~~(Math.random() * 100_000) + 500_000;
    const clustersPerBlock = ~~(CLUSTERS / ScandiskSurface.NUM_BLOCKS);
    return clustersPerBlock
  }
}

Luego evitaría el ~~ en favor de una función de utilidad, normalmente ese tipo de coerciones no son tan claras para quien las lee.

Aprovecharía y ya que tienes Babel discerniría entre métodos públicos y privados usando el "#" usando este plugin. Es seguro ya que esa propuesta entrará en la revisión es EcmaScript 2020. Además, colocaría los públicos arriba del todo y los privados abajo ya que los más importantes son los públicos.

Un constructor que está vacío no hace falta (aun incluso si extiendes de otra clase) como aquí, así que lo podrías borrar.

En esta parte podrías favorecer la immutabilidad:

constructor() {
    super();
    this.blocks = this.getSurfaceBlocks();
}

genSurface() {
    return NUM_BLOCKS.map(block => new ScandiskSurfaceBlock())
}

get surfaceBlocks() {
    return this.blocks.map(item => item);
}

Y la función surfaceBlocks, si no me equivoco, no hace nada, así que la podrías borrar.

En estos casos yo aprovecharía lo nuevo de JavaScript para simplificar:

constructor() {
    super();
    this.readFlag = "";
    this.type = this.type ?? this.getType();
  }

getType() {
    const randomRange = ~~(Math.random() * 5);
    return ["unused", "unused", "unused", "used", "full"][randomRange];
}

Y aprovecharía evitaría el uso de acrónimos y siglas en código. Acuérdate que las coerciones de tipo ! sobre valores no booleanos no son seguras, por ejemplo cuando hacemos coerción de !"" es falsy, al igual que !0. Para eso se introdujo justamente el Nullish coalescing operator

En casos donde se usen valores numéricos que representan constantes yo les daría nombre como en este caso:

checkBadBlock() {
   const blockIsBad = 1
    if (random(1, 150) === blockIsBad) {
      this.type = "bad";
      return random(2000, 4000);
    }

    this.readFlag = "read";
    const blockIsOk = 0
    return blockIsOk;
  }

Además, el nombre si además de hacer una comprobación devuelve un valor yo cambiaría el nombre del método para incluir el prefijo get o similar.

En casos donde tengamos getters y setters que no tengan lógica borrarlos como ocurre aquí. En Java y en otros lenguajes es necesario porque si quisieses añadir lógica de seteo no podrías cambiar de un campo a un método fácilmente. En JavaScript es trivial, ya que añades get o set a un método y listo.

Y por último diría de revisar si realmente tu lógica de negocio debería estar en los componentes y no en algún lugar agnóstico al framework. Sobre todo temas de cálculo y gestión de aleatorios. Los componentes no deberían saber nada de eso. Además, sacar eso fuera va a hacer que sea más fácil testear tus componentes.

Y por supuesto harían falta incluir test unitarios tanto de los componentes como de la lógica agnóstica a los componentes. Para los componentes te recomiendo Testing Library.

Muchas gracias por tu tiempo y por haber dejado que hiciese esta revisión ☺️

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.