uspdev / copaco

COntrole de PArque COmputacional
8 stars 14 forks source link

Melhoria no código que encontra o próximo IP. #325

Closed wgnann closed 5 years ago

wgnann commented 5 years ago

Troquei de ISO-8859-1 para UTF-8 também.

thiagogomesverissimo commented 5 years ago

Nice! Amanhã vou testar!

girol commented 5 years ago

@wgnann não tive tempo de revisar mandando PR :finnadie: . Mas fica aqui os pontos que achei relevantes comentar:


 $netmask = ~0 << (32-$cidr);

Eu adicionaria um comentário colocando a entrada e a saída desse operador bitwise, só pra relembrar. Eu mesmo sempre tenho que olhar a documentação pra entender o que ele faz.

thiagogomesverissimo commented 5 years ago

Dado que do ponto de vista da lógica o código funciona e sim o desempenho ficou melhor aprovei o PR e essas questões de estilo de código vou deixar para outra PR. Se @wgnann quiser, podemos fazê-la juntos.

thiagogomesverissimo commented 5 years ago

@wgnann ,

O e-mail que está no seu .git/config da sua máquina está diferente do que você usa no github. Assim, os commits não estão ligados com sua conta do github (https://github.com/uspdev/copaco/commits/master). Isso não implica em nada sério, mas você não aparece como contribuidor do projeto em https://github.com/uspdev/copaco/graphs/contributors por conta disso.

wgnann commented 5 years ago

@thiagogomesverissimo sobre o autoload, na realidade eu meio que fui pegando o que precisava para rodar as coisas. não entendo nada das coisas do laravel aí não sei como ele determina os paths e coisas assim, daí resolvi fazer as coisas do jeito mais simples (a.k.a. incluir as dependências até funcionar).

sobre o email, é suficiente eu trocar no .gitconfig? (vocês gostam dessas métricas de contribuição? para mim tanto faz, na realidade)

@girol sim. o teste que eu desenhei é para lidar com o caso do espaço de IPs que temos aqui no instituto (o que motivou eu escrever). acho que podemos meramente gerar uma lista enorme de IPs (pode ser até aleatória, inclusive) e substituir o teste do "mundo real" que fiz para ver o que aconteceria com a rede daqui.

quanto ao nome, para mim tanto faz. é que broadcast() pareceu a menor palavra capaz de condensar a ideia de encontrar um broadcast dado um ip e um cidr. ainda nesse sentido, podemos transformar o arcano de operadores bitwise (são os mesmos do C, por isso que eu lembro fácil) noutro método e usar verbocidr2netmask() em vez da variável.

em relação ao ganho de desempenho, o que muda é que em vez de fazermos diversas operações de potência de 2 para mexer com os bits, mexemos com os bits. pareceu mais natural pensar que IP é uma sequência de bits do que uma soma de potências de 2. na prática, o shift substitui um for feio, mas ambos gastam O(1).

girol commented 5 years ago

@wgnann , cara, bem massa!

Sobre os testes: não tem problema ser um caso real seu. Eu recomendaria o seguinte:

Criar um teste novo, com o nome tipo: TesteRedeDaUnidadeDoWill

E lá dentro seguir os padrões dos testes do Laravel com PHPunit. Dentro da pasta /tests/ tem uns exemplos. Tem um pindura inteiro sobre isso tb :smile:

Sobre os includes, basta usar os namespaces com o verbo use:

# esta linha:
require_once('../app/Utils/NetworkOps.php');

# Trocar por esta:
use NetworkOps; // Está no mesmo namespace declarado no topo

# estas linhas:
require_once('../vendor/s1lentium/iptools/src/PropertyTrait.php');
require_once('../vendor/s1lentium/iptools/src/IP.php');
require_once('../vendor/s1lentium/iptools/src/Network.php');
require_once('../vendor/s1lentium/iptools/src/Range.php');

# Trocar por:
use IPTools\IP;
use IPTools\Network;
use IPTools\Range;
use IPTools\PropertyTrait;

Sobre os operadores, eu realmente sou muito ruim nisso, mas sua explicação fez muito sentido :smile_cat: