yegor256 / quiz

Refactor the code to make it look more object-oriented and maintainable

Imagine you're assigned as a developer to this project and you have to make the first step in the refactoring of one of these classes (choose one, by the language you're most comfortable with). There are many issues in these classes, even though they compile and work.

Please, submit a pull request with the changes you would recommend to do here. Try to spend 15 minutes maximum for the entire work. If you need more time, you're doing something wrong. Remember, you have to do just the first step in refactoring. Don't fix everything.

Also, if you think that something else must be refactored further, put your ideas into the description of the pull request.

Thanks!

PS. You may want to watch this lecture too.

quiz's Issues

Link to join page is incorrect

Please update the link to http://at.teamed.io/join.html

Recomendations for parser

bad practice
//code agains implementations
FileInputStream i = new FileInputStream(file);

better practice
//code agains interfaces
InputStream i = new FileInputStream(file);

bad practice
//using string class for concat
better practice use string builder or Stringbuffer syncronized
for concat operations String class has a Big O linear time complexity because String class create a new String object every concatenation because
String is an inmutable class and with StringBuilder has Big O constant time complexity StringBuilder is a mutable class and also for concat is better
because if you use the append method this will be adding the string and the end of the String Builder so thats why it has a Big O constant time complexity

//Also missing try catch block for IO operations and closing the stream in the finally block

//use syncronized block instead of syncronized method, syncronized methods locks the whole class

//also this should be an interface parse interface and the concrete implementations of diferents kinds of parsers using decorator
design pattern something similar to the JAVA IO decorator

InputStream in = new BufferedInputStream(new FileInputStream("test.txt"));

Or maybe use strategy or template pattern instead if is planning to have more parsers in the future.

Document.php

There are two obvious SQL injections in there.

Doing a little quiz.

I was too lazy to make some code but here are the things I noticed:

• Why is the class called parser? It does not parse anything, it provides functionality to read and write stuff from a file.
• The class violates SRP: It performs file IO and charset filtering.
• The class is mutable which is a major source for all sorts of weird bugs.
• Encapsulation problem: setFile/getFile seems bad design, immutable object would be better.
• Why is getFile needed at all? Should be removed if possible.
• I'm not sure why thread-safefy is announced in the javadoc. What exactly should be thread-safe here? Nothing prevents two threads from simultaneously write to the same file and f.. up things.
• Reading and writing does implicit binary-string conversion but I can not tell it which encoding to use.
• Reading large files will take a long time because (a) no byte buffer is used and (b) += operator is used which will create a new string instance each time IIRC.
• InputStreams are not closed and will leak resources. try-with-statement should be used.
• getContentWithoutUnicode(): Should this really be named unicode? I'm no expert in Unicode but I think this should be named something like getContentWithoutUTF8Characters() or something. Also, Unicode is a standard, UTF-8 is a character encoding :)
• I am not sure what the intent for getContentWithoutUnicode() is but filtering for 0x80 seems weird. My wtf sense is tingeling.
• Unit tests are missing.
• No way to mock the filesystem.
• Javadoc does not describe what the class should actually do.

For the refactoring, why not delete the class and use Files.readAllBytes() and Files.write() instead? Given that the code is so bad, the rest of the project might be too and if we are really lucky getContentWithoutUnicode() is not used at all and then we don't even have to build a new class. :-P

I/O Streams should be "final" in Parser

These streams are created for single task. final them to make it clear and safer.

Concurrency issues in Parser

Is it OK to change interface?

Im seeing that some people rename method names and input arguments. Is it OK in the context of this quiz?

By definition, refactoring means that interface does not change. So I'm wondering whether it's something you want your candidates to figure out or you actually expect that the interface will be changed for (maybe) a better one?