JujaLabs / itevents

Resource to subscribe on it events
Apache License 2.0
7 stars 5 forks source link

#214=fix-pmd-crawler #216

Closed vaa25 closed 8 years ago

vaa25 commented 8 years ago

connect to #214

romach commented 8 years ago

@vaa25 you can temporary add your branches for Travis integration in .travis.yml here:

branches:
  only:
  - master

Later when you merge to master you will delete them.

romach commented 8 years ago

@vaa25 It is better to add .properties files check like here and here

IgorMaksymov commented 8 years ago

@vaa25 base branch for this is master or #214=fix-pmd-crawler ?

alex-anakin commented 8 years ago

Disclaimer: I review all code of crawler in this pull request.

  1. HttpFetcher needs a contract (interface).
  2. HttpFetcher.fetchAsString is too complicated. Use private methods inside try block.
  3. You throw the same exception with different messages inside HttpFetcher.fetchAsString. It's a logic so it should be tested (you are need check the text of exception messages).
  4. Why EngineObserver is handleEvent? It's non-obvious.
  5. Class Engine implements EngineObserver and has public method run. But EngineObserver hasn't method run.
  6. I don't like your class StringLoader. It is hard to reading and understanding. I will explain you how it needs to be implemented by example.
public interface StringFromSource {
    String value();
}
public class StringFromFile implements StringFromSource {

    private final String value;

    public StringFromFile(final String path) {
        // load file etc.
        this.value = ... ; // make new String from file bytes
    }

    @Override
    public final String value() {
        return this.value;
    }
}

getting string value of file:

String s = new StringFromFile("d:/file.txt").value();`

It's easy, isn't it?

vaa25 commented 8 years ago
  1. Why?
  2. Will be fixed.
  3. Will be fixed
  4. Will be changed in #213.
  5. Because Engine is not only EngineObserver, but it has other logic also.
  6. Why value() instead getValue()?
alex-anakin commented 8 years ago
  1. So you don't know why do we need the interface? It's your choice.
  2. Ok
  3. Ok
  4. Ok
  5. So it doesn't align SOLID principles.
  6. It does not matter. Use getValue() if you like it.
vaa25 commented 8 years ago

@AndriyBaibak 4h