Собственно ребята. Написал прогу для загрузки картинок в группу в vk.com. Это вторая написанная мной прога в процесе изучения java. Хотелось бы услышать критику от более опытных людей, если конечно вам не очень влом ковыряться в чужом коде.
Програмулину выложил на гитхаб https://github.com/GaraZ/VKLoader
еще есть архив если влом заливать

Заранее благодарен за участие )

ЗЫ: в предыдущей теме прогнал со ссылкой ((

Blazkowicz,

спасибо огромное за коментарии!

BlazkowiczApp.
- Java logger никто не юзает. Даже тут видно что его не удобно конфигурировать.

а что юзают? Log4J?

Blazkowicz"Архитекура на Helper-ах" показывает несостоятельность в следовании Single Responsibility Principle. Если класс называется Helper, это значит что его создатель понятия не имеет для чего этот класс ему будет нужен. Так же и для читателя слово Helper никакой полезной информации не несёт. Просто мусор в коде.

Вы правы. "Архитекура на Helper-ах" - я понятия не имею что это какая то специфическая архитектура. "Helper" назвал без какойто задней мысли

BlazkowiczmainForm.setVisible(true) вызывается до инициализации. Это значит юзер увидит не валидную форму. Если в методе initSettings предполагается progress indicator, то я бы назвал его runInitialization. Если не предполагается, то инициализацию стоит выполнить до вывода на UI.

Не доглядел. В принципе согласен.

BlazkowiczCommon
- класс с говорящим именем. Сразу понятно о чем речь. Ещё и с volatile полями

Ну да )) Название наверное не очень. Там хранятся общие настройки. а volatile потому что эти настройки могут дергать несколько потоков
Хотя да скорре всего volatile в данном случае бесмысленно.

BlazkowiczЯ бы не рекомендовал user.dir. Он может быть разным у одного и того же приложения. В зависимости от того как запущено приложение.

Можете этот момент разъяснить более подробно?

BlazkowiczНет такого слова uncorrect. Возможно такое словообразование, но оно обозначало бы совсем другое.

Там должно быть incorrect. Провтыкал


BlazkowiczgetArhContentDir и getArhPagesDir бессмысленные методы. Созданые через copy paste. Если конфигурация не валидна, то приложение должно говорить об этом на этапе инициализации, а не на этапе использования настроек.

Ну я хотел обезопаситься, если пути стали некорректными в процесе работы приложения. Ведь путь можно задать какой угодно в любой момент. Хотя возможно стоило поступить как-то иначе

BlazkowiczВместо mkdir() стоит использовать mkdirs(), чтобы обезопасить код от будущих изменений в настройках.

Да что то я этот метод провтыкал. Так конечно лучше.

Blazkowiczдля простого XML я бы использовал JAXB, а не DOM. Тем более что вам в обе стороны сериализовать нужно. DOM подходит хорошо для изменения данных в существующем XML.

Я про JAXB не слышал. Поковыряюсь что за зверь.

BlazkowiczgetKey() - создаём массив значений, но зачем-то их все потом заменяем. Честно говоря не понял задумки. Имя метода плохое - не сообщает что именно делает метод. Так же в этом методе стоило написать коментарий. Мне сходу не понятно что там происходит.


Ну задумка в том чтобы ключ зависел от мак адреса. Но поскольку длинна массива батов должна быть 16 байт, а длинна мак адреса меньше, то я заменяю только первые несколько байт в первоначальной последовательности
Ну здесь конечно есть недоработка если мак адресов несколько)
статические они потому что для каждой шифровки/дешифровки не вижу смысла создавать экземпляр класса.

Blazkowicz- Нельзя так писать.
if(source == null || source.trim().isEmpty())
return null;


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


Насчет скобок согласен. Провтыкал просто. Ну а насчет нула - возможно вы правы. Подумаю.

BlazkowiczCustomList
- ещё одно имя не о чем. Можно было называть хотя бы XmlList. Правда не ясно почему обязательно эти методы к List привязывать, а не к любой сущности.
- Зачем там E extends Object тоже не ясно.

С фантазией что то туго было на тот момент. Ваше название получше. )) E extends Object не нужно действительно. Чесно говоря не помню о чем я тогда думал


BlazkowiczExceptionLoggerForm
- Хорошая попытка дать внятное имя классу. Но тем неменее не удачная. Почему Exception, если туда можно всё логировать? Почему Form? Если оно не для ввода, а для вывода.

Ну хоть гдето хорошая попытка )))

Blazkowiczto be continued...

Буду ждать )

mayton,

перерыл весь код
где я его использую java_cup так и не нашел )

Blazkowicz,

Blazkowiczjava -jar {полный путь к jar приложния} - эту команду можно запустить из любой директории системы. Из какой запустите, та и будет user.dir.
Есть ещё такая тема что в Windows приложения принято ставить в Program Files. А в корпоративном\государственном секторе админы могут запретить запись в Program Files. Если ваше приложение захочет там хранить файлы, то он не будет работать под пользовательским аккаунтом. Только под админом.

Именно изз того что доступ к папке может быть залочен я решил по умолчанию складывать все туда где запущено приложение
про то что приложение можна запустить с любого места я както не подумал
так какое наиболее приемлимое решение?

GaraZBlazkowiczCustomList
- ещё одно имя не о чем. Можно было называть хотя бы XmlList. Правда не ясно почему обязательно эти методы к List привязывать, а не к любой сущности.
- Зачем там E extends Object тоже не ясно.

С фантазией что то туго было на тот момент. Ваше название получше. )) E extends Object не нужно действительно. Чесно говоря не помню о чем я тогда думал


Я вспомнил ))
без объявления
в интерфейсе я не могу сделать
вот так в реализации


а без этого я не могу сделать вот так

Blazkowicz,

Blazkowicz- Родительское окно не указано для JDialog, это приводит к багам в Swing при пеперключении через Alt-TAB между окнами. Особенно на модальном может вылезти. Лучше указывать родительское окно всегда.
- Я вообще не понимаю, почему вы учитесь на Swing. Стоило взять JavaFX.

Багов не замечал. Компоненты Swing описывались в тех учебниках которые я читал. Про JavaFX ничего не встречал, а специально не смотрел. Посмотрю по свободе что за зверь

Blazkowicz- В данном тривиальном примере, конечно, не критично. Но переменным всегда стоит давать внятные и красивые имена, даже UI контролам. А не те что предлагает IDE по-умолчанию.
textArea, mainPanel, scrollPane читаются и выглядят намного лучше чем jTextArea, jPanelMain и jScrollPane.

Интерфейсы я создавал вручную.Эти названия мне показались достаточно красивыми ))

Blazkowicz- MASKS_LIST - почему константа и вдруг Mutable список?
- if (MASKS_LIST.isEmpty()) return true; Зачем проверка что он пустой? Другие классы меняют состояние этого поля? Инкапсуляция ни к черту.

Да в данном варианте согласен - полная ерунда. Я собирался MASKS_LIST вынести в интерфейс, но потом чето передумал, а переписать забыл.
Гон конечно. Каюсь (

BlazkowiczВам же уже писали что JavaDoc стоит читать, а не копипастить код отовсюду.
- getHarmonizeSelFiles - генерики куда-то просрались.
- if (fl == true) - Идия Style?
- fl это вообще что за переменная? Flag? Flag чего? Совпадения? Так и надо было назвать тогда equal, matches, etc.

Доку читаю по необходимости, но копипастить проще )))
Про fl наверное следовало назвать более осмысленно, но функция маленькая в ней сложно запутаться

Blazkowiczfile.delete() - результат этого метода стоит обрабатывать. А так же изучть Java NIO2 API для работы с файлами. И использовать его.

Спасиб посмотрю.

Blazkowicz- используйте Buffered UI Stream-ы. Это проще чем самому через byte[] гонять данные.

Так проще или лучше?

BlazkowiczGregorianCalendar calendar = new GregorianCalendar(); лучше Calendar calendar = Calendar.newInstance();

Чем лучше?

BlazkowiczЛагманпропущено...

это из-за того что копипастят из декомпилорованного кода.
Не думаю. Просто прочитали что конкатенация тормозит, но не разобрались где и почему.

именно ))

Blazkowicz,

BlazkowiczJCanvas
- в drawImage вместо null стоило передать this. Это должно уберечь от частичной отрисовки изображения до его окончательной загрузки.

Спасибо не знал

Blazkowiczесли мне влом подключать Spring или другой IoC контейнер, я просто завожу отдельный класс, который у меня собирает всю структуру. Такие циклические зависимости как App создаёт MainForm, а MainForm использует App, только всё усложнают. Аналогично и с SettingsForm.

Да согласен что както запутано. Но как будет лучше я както смутно представляю

BlazkowiczFilesTreeCellRenderer вызывает showImage, который больше нигде не используется, но метод зачем-то во внешнем классе. Renderer, ведь, можно было спокойно отдельным классом оформить?

Можно было конечно

BlazkowiczСтроки вида app.getSettingsHelper().getCommon().getIsArhPages() - это грубейшее нарушение инкапсуляции. А так же "угадай какой из методов верунул null" и привел к NPE.

а как лучше сделать? не испотльзовать один класс дя доступа к другим а передавать их все в конструктор (Допустим MainForm)

BlazkowiczВместо if (evt.getSource() == someControl), вместо KeyAdapter и т.п. лучше использовать Action. Его можно пере-использовать. Из них проще строить иерархию. Они не будут раздувать методы, когда у вас появиться десяток-другой пунктов в меню. Плюсов море.
MigLayout рулит.

Погляжу. Пока я не в теме

BlazkowiczgetFocusDown() это для чего вообще? Есть устойчивое чуство что можно было проще.

Когда из Дерева удаляем ноду - фокус должен переместится на следущую или на первую - если удаленная нода последняя

Blazkowiczline.separator разве работает в JOptionPane? По-моему нужно JTextArea использовать вместо строки

а почему нет


BlazkowiczProfileObj
- Object login; - как так?
- А вот это оригинально.
public boolean equals(Object obj) {
...
return this.hashCode() == obj.hashCode();
}

В общем в текущей реализации переопределять equals вообще нет никакого смыла. Это осталось от более ранней версии но я и сам понял что херня какаято
вообще забыл что что-то там переопределял ((

Blazkowicz,

BlazkowiczreadFile не закрывает стримы. Почему try-with-resource не используется?

провтыкал (

BlazkowiczSiteHelper
- длинный список аргументов я предпочитаю форматировать в столбик.
- GetThread - ни о чем. Что за поток? Чем занимается?
- GetThread - цикла в run не вижу. Зачем надо было наследоваться от Thread? Используйте Executors/SwingWorker.
- Есть потоки, но обращение к GUI не выполнено из EDT. Срочно читать статьи о потоках в Swing.
- parse методы - каша, вникать лень.

Поток парсит страницу выдирая с нее картинки
Для каждой страницы в runSitesDownload(CustomList<SiteObj> sitesList)
создается свой поток GetThread
Цикл в run и не нужен


Blazkowiczclass Timer implements Runnable
Really? То есть java.util.Timer, javax.swing.Timer и ScheduledExecutorService на столько кривые, что пришлось написать свой?

Ничем мой класс не лучше. Просто мне нужно было и jLabelTime (Циферблат) менять каждую секунду и еще и задание выполнить через n секунд
После беглого просмотра по существующим классам понял что для этого придется применить два таймера
Ну может это и не так. Чесно говоря сильно не вникал.

BlazkowiczПо-тому что jlabel не умеет показывать многострочный текст, без <html>.

Ну незнаю )0 у меня все переносит

MasterZivBlazkowicz,

а оно работает ? Использовать можно по назначению?

Если вас интересует работает ли приложение - то да вполне
я каждый день его юзаю для своей группы
Мне интересно было бы если бы ктото еще попытался его заюзать )))

Basil A. SidorovmaytonМожно проверить версию ОС и использовать тильду или %USER_PROFILE%Зачем, если есть стандартное свойство?
какое стандартное средство?

GaraZBasil A. Sidorovпропущено...
Зачем, если есть стандартное свойство?
какое стандартное средство?
свойство тоесть

Basil A. SidorovGaraZпропущено...
свойство тоестьСмотрите вывод:
если уж из топика непонятно.
ааа
я подумал еще чето есть )

0FDGaraZ,

SiteHelper :
private static Map<URI, byte[]> pageMap;
private static Map<URI, byte[]> imgMap;
private static List<Exception> exList;

Используются несколькими потоками GetThread
ну да
что не так?

0FD, Лагман,
нежданчик
я даже и не подумал посмотреть являются ли они потоко-безопасными
явный прогон с моей стороны
спасибо )

Постарался учесть изложенные выше замечания
Обновил репозиторий :)