Hi all, had to double check and it took quite a while till I noticed that you were talking about the generify branch ;-)
I thought this had already been merged and just noticed it hadn’t. Perhaps it’s good this way as it seems we should probably discuss things in the team first. Thinking more about the topic, I don’t think single-value and batch should be the discriminator on this topic, but single-type and multi-type. For single-type we could use generics to make casting obsolete, but is that worth the more complicated API? Right now (but that’s just my opinion) I would prefer the simple (non-generic) Read/Write Request/Response but to have generic Items. These generic Read/Write items could be used in conjunction with a generic getValue in the response class that returns the corresponding generic type of ReadResponseItem (in case of the write response, the response doesn’t require a generic type). I do agree that the API should be as lightweight as possible. That was the reason for me removing all generic types 1 week before Christmas. But then I had not only generic types, but I also had to provide 2-3 classes per supported type. Getting rid of all of these classes was definitely a good choice. While I was at it, thinking mainly about optimized read operations (I guess most use-cases will be reading multiple values in one request), I thought the added complexity of having generic and non-generic classes/methods did not overweigh the increased complexity, so I removed the generic support completely. Regarding the Builder/Direct Instantiation … what would be the benefit of providing a builder and preventing direct instantiation? I think, giving the user a choice is always something good. There are people that prefer builders and some don’t. We should support both (I think). Perhaps it would have saved us a lot of time, if we had managed to really meet and do a proper API kick-off. Chris Am 07.01.18, 23:52 schrieb "Mark Keinhörster" <[email protected]>: Good evening, short update on the generify: -> I added the getValue-method as proposed to the bulk requests in the generify branch. -> added unittests -> noticed that the old unittests were not working and fixed them (that was quite unfortunate) -> found a bug in the builder at PlcReadRequest, the initial check in „checkType“ should look if the type is null instead of != otherwise it will never be set. private void checkType(Class dataType) { if (firstType == null) { firstType = dataType; } if (firstType != dataType) { mixed = true; } } —> changed one unittest to actually using the builder but this needs more testing if we stick to this Feedback: Overall it feels quite heavy with all these options: —> Builders and direct instantiations —> checked, unchecked, bulk, single requests/responses To the first point I personally would not favor a any of the two but either stick with one or the other and not both. To the second point, we can discuss what’s more suitable. I would prefer the unchecked version with „getValue“ but I am open to any argument/recommendation against it. What do you think? Enjoy the evening, Mark > Am 05.01.2018 um 13:43 schrieb Sebastian Rühl <[email protected]>: > > Hi Together, > > I refactored the branch (refactoring/java_generify) a bit so that you now can do following: > > PlcReadRequest req = new BulkPlcReadRequest() > req.addItem(new ReadRequestItem<>(Byte.class, inputs)); > req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3)); > CompletableFuture future = plcReader.read(req); > PlcReadResponse resp = (PlcReadResponse) future.get(); > Only difference on the requirement from Chris is, that you now need a specific request implementation and add the <> onto the Request Items. > Currently following Types are implemented (PlcReadRequest* is a interface now): > BulkPlc(Read|Write)Re(quest|ponse) > CheckedBulkPlc(Read|Write)Re(quest|ponse) > SinglePlc(Read|Write)Re(quest|ponse) > > The tag of the pre-refacotring is „pre_bulk_refactoring“ if needed for comparison (On branch refactoring/java_generify). > > Essential is the hierarchy of the implementation now: > > PlcReadRequest > / \ > SinglePlcReadRequest BulkPlcReadRequest > | > CheckedBulkPlcReadRequest > > With this following Signatures can be used to conveniently work with strong typing: > > public interface PlcReader { > > CompletableFuture<? extends PlcReadResponse> read(PlcReadRequest readRequest); > > @SuppressWarnings("unchecked") > default <T> CompletableFuture<SinglePlcReadResponse<T>> read(SinglePlcReadRequest<T> readRequest) { > return (CompletableFuture<SinglePlcReadResponse<T>>) read((PlcReadRequest) readRequest); > } > > @SuppressWarnings("unchecked") > default CompletableFuture<BulkPlcReadResponse> read(BulkPlcReadRequest readRequest) { > return (CompletableFuture<BulkPlcReadResponse>) read((PlcReadRequest) readRequest); > } > > @SuppressWarnings("unchecked") > default <T> CompletableFuture<CheckedBulkPlcReadResponse<T>> read(CheckedBulkPlcReadRequest<T> readRequest) { > return (CompletableFuture<CheckedBulkPlcReadResponse<T>>) read((PlcReadRequest) readRequest); > } > > } > > > The mentioned hierarchy is essential to make the overloaded methods work. > > As always Im happy about feedback :) > > Sebastian
