Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLE communication refactoring #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

BLE communication refactoring #7

wants to merge 5 commits into from

Conversation

monday8am
Copy link
Contributor

Hint for reviewer:

This code works right now. The next topic is to decide where to include it (as part of our lib or as a demo implementation)

@monday8am monday8am requested a review from sergkh June 6, 2017 12:38
package com.fidesmo.ble.client.apdu;


public interface CustomPacketDefragmenter {
Copy link
Member

@sergkh sergkh Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can think on how to merge this CustomPacketDefragmenter with original Packet fragmenter/defragmenter interfaces? Some of those methods can be handy there too.

But I have some notes about the interface:

  • Method add is not informative, looking at the interface you can't say what it is doing and how it is different from the append. Maybe we can rename both to something like addPacket and addBuffer (or nay other variant)?
  • Interface is not consistent: you have Java Bean style getBuffer and simplified complete (which in turn has to be completed maybe?) and empty. IHMO it would be good to choose one naming scheme or append isCompleted or remove get what is better to you.
  • Original intent was to not use clear and simply recreate the builder, why do you want to reuse it?
  • Github complains about not having newline at the end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants