filiphajek
commented
1 year ago Hello, we want to address or explain your concerns/issues.
Of course, we do not want to have 25 classes. But yes we are aware that this should be reported in our pdf much better. We should have explained that this is not a plan for the final deadline .. and yes our code from phase II and report looked like we are going for 25 classes.
Right from the beginning we had an idea that we would be reusing converters (commit '6d058afe097b226e042873dc73c1dddb3ebf6b43') .. just like we are doing now - https://github.com/adamzalesak/PV286-Project/pull/23. The reason why we ditch the idea was that we were not sure what would be the best common format, so we agreed to start implementing and come back to this idea later when we would have enough information to use this approach. We were also considering some 'common custom format'. But we agreed that reusing converters will be simpler than implementing our format/representation. We also agreed that there are lots of unknowns so we just start implementing converters basically for every combination and we choose some kind of iteration strategy = let's implement something, get some result, analyze it, and refactor it if needed. The reason why we choose this is that we were not moving towards a successful end before. We eventually were in a situation where we prove to ourselves that reusing converters is the way to go but we agreed that the main focus is to meet the conditions of phase II because the deadline was getting close .. so we implemented the rest of the converters no matter what. We agree that what we submitted was not ideal .. especially the design of using one convertor for one format-to-format combination. But I would not it describe as too bad. At least implementations were separated so that the code was easy to refactor into the current solution. Also from the first commit, the high level of the design was that we would try to separate concerns so that there will always be ArgumentParser, ConvertorFactory, and convertor classes. This design is going to remain. What we changed is that there is a new CommonConvertor which also implements IConverter and takes two existing converters. The first convertor converts the source to bytes and the second one converts to the target format. The converters from phase II remained unchanged, but some converters were deleted, because we did not need them anymore. I think that our bigger mistake was the base converter class. This class does not exist now .. well it is still there only renamed as 'ConvertorDirector' (no longer under the IConvertor interface) which is controlling the flow of reading source stream, checking delimiter, etc. The real conversions implement classes of the IConvertor interface. This interface is passed down to ConvertorDirector from ConvertorFactory. The ConvertorDirector class only calls the IConvertor method ConvertPart when the delimiter appears or there is the end of the stream.
We agreed that we underestimate not just the preparation part but maybe the whole project and that we should take some steps differently. We should not have finished it by the day of the deadline (we should have started sooner). Also, we implement features that we did not have to deliver, we should have focused more on delivering the final codebase design with proper testing. The last thing is that we planned to deliver a product on a hard deadline, not focusing on the first deadline - another mistake.
Hello @adamzalesak, @filiphajek and @xplatk.
Sorry for the delayed project review.
I will use issues for suggestions, problems and improvements. Feel free to comment on any of these issues. This one is dedicated to the report.
On the high level, the report makes sense, but there I have some issues with what is being reported.
Do you seriously want to have 25 classes dealing with all possible combinations of inputs and outputs?
Surely there are better options, for example, choosing an Intermediate Representation and then having just two classes per format. One is to convert input into IR, and the other is to convert IR into output. Instead of n² classes, you would only need 2n.
The argumentation that TDD is useless because it broke when you changed the code points to problems on your part, honestly.
Of course, sometimes changes are necessary; nobody writes the perfect code from the start. But considering the problem before writing any code, identifying problematic areas and trying to decompose the problem first could minimise the need for changes.
The fact that you needed to change a lot of code after it had been initially written (and the “all combinations” point above) suggests that you might have severely underestimated the preparation part.
Setting up Continuous Delivery/Deployment to create Nuget packages is okay, but perhaps you should have spent this time rather on the Unit tests.
The entire review is in the attached PDF file as annotations.
report-review.pdf