GaraZ,

В Config.crypt поток читается не правильно. in.read может считать один байт даже до конца файла. Так что вы завершите операцию раньше, чем надо.
Config.crypter занимается перекладыванием байтов в потоки и обратно. Он вместе с crypt может быть переписан в один doFinal.
Config.encrypt/Config.decrypt в случае исключений возвращают пустое значение. Здесь нужно какой-нибудь RuntimeException или Error (например, AssertionError) бросать. А так вы молча теряете данные.
В Config.setProfiles смущают String.valueOf(...). Это чтобы null обработать? В этом случае вы строку "null" и значение null запишете одинаково.
Config.getProfiles. Проверка на fileExists - первый шаг к уязвимости DoS. Файл может быть удален между проверкой и последующим чтением. В подобных операциях стоит сразу открывать файл и затем отдельно обрабатывать FileNotFoundException.
ReportApi.getDir как-то странно рекурсию использует и игнорирует возвращаемое значение. Его стоит переписать нерекурсивно (для вашего случая) или исправить реализацию. А еще посмотрите API у File. В частности, методы list* и конструкторы.
Reports.getReportList совершенно неожиданно изменяет внутреннее состояние класса.
Ну и для большого приложения в каждом из сервисов стоит сделать свои исключения, а подробности реализации вроде SQLException оборачивать в них.

GaraZmaxkarВ Config.crypt поток читается не правильно. in.read может считать один байт даже до конца файла. Так что вы завершите операцию раньше, чем надо.

С этим замечение не согласен. Конечно in.read может считать до конца, но метод cipher.update вызывается для полноразмерных блоков данных, а метод cipher.doFinal для неполных блоков (которые сам дописывает). Поправте если я не прав.


Здесь сразу два момента. Во-первых, даже блочные шифры не требуют кормить ему данные размером с блок. "Блочность" шифра - это некоторые характеристики внутри алгоритма, они не связаны с тем, как правильно вызывать этот алгоритм. И ни один из методов update не выбрасывает исключения "недостаточно входных данных". Если вдруг данных будет недостаточно, шифр запомнит введенные данные и при следующем вызове их тоже использует. Т.е. вы их получите обратно, когда скормите достаточно данных. Косвенно это подтверждается и в документации по Cipher.getOutputSize ("This call takes into account any unprocessed (buffered) data from a previous update call, padding, and AEAD tagging."). Т.е. данные могут буферизоваться. Отсюда же следует, что на самом деле getOutputSize желательно вызывать на каждой итерации и при необходимости создавать новый массив (мало ли что там забуферизовалось).

Второй момент. Даже если предположить, что шифру данные нужно давать порциями в blockSize, читаете вы их все равно неверно. Даже при blockSize=5 и 10 байтах в потоке InputStream.read может считать только один байт. В этом случае ваш код пойдет делать doFinal, а это слишком рано. Нужно реализовывать код, который в любом случае пытается вычитать массив "по-максимуму".

GaraZmaxkarConfig.crypter занимается перекладыванием байтов в потоки и обратно. Он вместе с crypt может быть переписан в один doFinal.

Не совсем понял. Crypt.crypter занимается тем что шифрует/дешифрует массив байт в зависимости от значения aMode(ENCRYPT_MODE/DECRYPT_MODE). doFinal метод Cipher, зачем мне его переопределять?

Я про взаимодействие crypter и crypt. Вот у вас в crypter байты заворачиваются в поток и вызывается crypt. И все это происходит после создания шифра. Вместо вызова crypt можно вызывать cypher.doFinal(aBytes), что сразу дает результат и избавляет от необходимости перекладывать байты в потоки или собирать их из потока.

GaraZТак и задумывалось если возникает ошибка при шифровании/дешефровании (в данном случае пароля), то выводится пустое значение и пользователю необходимо ввести его вручную.

Я на месте пользователя буду совсем не рад, если из-за каких-то ошибок программа потеряет мои данные. Причем ошибки даже не мои. И все это происходит без предупреждения. Т.е. в правильном варианте хорошо бы сказать пользователю "шифрование не поддерживается, пароли не будут сохранены" и пусть уже он решает, стоит ли пользоваться приложением или нет. Или просто не запускать приложение, если алгоритмы недоступны.

Вот какие-то ошибки расшифровки можно игнорировать. Например, пользователь вручную изменил шифрованный пароль в конфиге. Тогда можно и сбросить данные. А в случае независящих от пользователя обстоятельств это не очень хорошо.

GaraZmaxkarВ Config.setProfiles смущают String.valueOf(...). Это чтобы null обработать? В этом случае вы строку "null" и значение null запишете одинаково.

К null это не имеет отношение. Этим кодом я просто гарантирую что в запишется именно тип String. Ну возможно это лишнее конечно.

Т.е. вы предполагаете, что в вашей Map там все-таки хранится не String? Ну так в этом случае очень желательно, чтобы приложение падало в случае ошибок (оно и упадет с ClassCastException если не вызывать String.valueOf) а затем разбираться, откуда у вас там в map могли попасть не те типы. Иначе вы потом концов ошибки не найдете (ошибка в одном месте, а падает совсем в другом).

GaraZmaxkarReports.getReportList совершенно неожиданно изменяет внутреннее состояние класса.

Не совсем понял. Можно поподробнее? Почему это совершенно неожиданно? Нужно было написать отдельный метод в котором бы заполнялось значение gRootDir?

Потому что из названия метода следует, что он только отчеты загружает. А на самом деле он еще что-то меняет. Пользователь класса же не просил изменять rootDir.

В идеальном случае метод либо не меняет состояния, но возвращает значение, либо изменяет состояние и ничего не возвращает (void method). На практике так не всегда получается (см. тот же Cipher). Но от get изменение внутреннего состояния совсем не ожидается.

Один из вариантов - да, выделить отдельный метод. Между прочим, этот метод у вас уже есть - initRootDir. Второй - инициализировать состояние только один раз (в конструкторе). Такой подход может быть не всегда применим. Третий - возвращать данные в объекте (например, там будут поля rootDir и reportList), а внутри класса состояние вообще не хранить (тем более, оно самим классом нигде не используется).


GaraZmaxkarНу и для большого приложения в каждом из сервисов стоит сделать свои исключения, а подробности реализации вроде SQLException оборачивать в них.

Не сосем понял о чем это вы. Можете подробней пояснить


Ну вот возьмем тот же Config. getProfiles/setProfiles выбрасывают всякие ошибки вроде SAXException и IOException. Т.е. любой пользователь привязан к деталям реализации. Допустим, потом вам XML надоест и вы решите использовать JSON. В этом случае добавятся еще исключение JSONException. При этом пользователь метода ничего в случае этих ошибок сделать не может. Он контролирует только набор данных. Вот IllegalArgumentException он может как-то обработать, а SAXException - нет.

Для "больших" приложений удобно заводить исключения в терминах "контракта" сервиса, а не его реализации. Вот у вас есть конфиг, который загружает/сохраняет профили. Поэтому при невозможности загрузить/сохранить профиль логичным будет что-нибудь вроде ProfileInaccessibleException. Это явно говорит пользователю метода "профиль недоступен". Исходное исключение (то же IOException) можно передавать как cause в конструктор ProfileInaccessibleException, так что все необходимые детали будут доступны, их можно будет сохранить в лог и т.д. Если вдруг вы измените формат на json, декларированное исключение у методов не изменится, изменится только реализация метода.

В дальнейшем вы можете развивать иерархию ошибок. Допустим, пользователи могут менять "профиль" вручную. В этом случае ситуации "при изменении испортили формат" и "у пользователя испортился жесткий диск" будут приниципиально разными. Можно будет ввести двух наследников ProfileInaccessibleException: ProfileUnreadableException и MailformedProfileException. Причем старый код тоже будет корректно работать. Т.е. свою иерархию исключений в некоторых пределах можно и под стоящие задачи подстраивать.

GaraZmaxkar,
maxkarВторой момент. Даже если предположить, что шифру данные нужно давать порциями в blockSize, читаете вы их все равно неверно. Даже при blockSize=5 и 10 байтах в потоке InputStream.read может считать только один байт. В этом случае ваш код пойдет делать doFinal, а это слишком рано. Нужно реализовывать код, который в любом случае пытается вычитать массив "по-максимуму".

Тут не согласен. Все прекрасно работает
int read(byte[] b)
Читает данные в байтовый массив и возвращает фактическое число прочитанных байтов или число -1, если обнаружен конец потока. Максимальноее количество байтов, которое может прочитать метод read(), определяется полем b.length

Оно работает только потому, что у вас ByteArrayInputStream. Этот Stream заполняет буфер по-максимуму всегда (когда есть данные). Для других реализаций InputStream подобное поведение не гарантировано. Т.е. отрицательное число - точно конец файла. Любое положительное число ничего кроме "прочитанны данные" не обозначает. Даже если read вернул число меньшее, чем b.length, это не обозначает, что был прочитан последний блок ввода. И в контракте этого не написано. А в строке 56 вы как раз ориентируетесь на "прочитался ли буфер полностью".

Чтобы получить ошибку, нужно другие потоки использовать. Можно попробовать файловые. А можно написать свой наследник FilterInputStream, который на любой read читает только один байт (это контракта не нарушит).