Код junior-а на C++
Просьба оценить то, что я сейчас пишу. Хочу подтвердить/опровергнуть свои мысли.
Программа C/WinAPI, работа с растрами. upload.com.ua/get/901736966/
Ambianx, инженер в НИИ
Facebook
Twitter
LinkedIn
Alexander Migal
>>> strcpy_s (szGroupPrcnt, 10, «0%» );
Лучше не указывать2-й параметр, если 1-й является массивом фиксированной длины. Шаблон функции делает это автоматически.
Лучше не указывать
Или использовать sizeof (szGroupPrcnt)
anonymous
qwerty,
Программа вообще недоработанная. Т.е., добавить некоторые функции, некоторые улучшить. Я уже и сам ее плоховато понимаю. Остался костяк, а на нем наворочены иногда не совсем качественно другие функции. Я банально позабывал куски. Программа писалась долго. Используется алгоритм быстрой сортировки цветов внутри групп. «Сбор информации» тоже не соптимизировать, а вот над группировкой надо подумать — оно совершает холостые проходы.
А почему ты используешь не штатную qsort?
Вот это «/* Предотвращение зависания приложения */» Встречается несколько раз, может как-то стоило бы заменить функцией?
Сама програма устойчиво периодически падает когда нажимать кнопку с картинкой arrow.bmp
Тебе удобно постоянно обращаться по полному пути структуры
PColors [dwIndex1].hls.wH такое постоянно, почему не сделать в начале что-то вроде
GPIXEL c = PColors [dwIndex1];
Ну и так далее.
Я плохо понял программу, но мне кажется SortSimilarHLS совсем лишня, ты же результат используешь для отображения в ImgSetupDlg
Тогда тебе достаточно информации о частоте появления цвета
Вот это «/* Предотвращение зависания приложения */» Встречается несколько раз, может как-то стоило бы заменить функцией?
Сама програма устойчиво периодически падает когда нажимать кнопку с картинкой arrow.bmp
Тебе удобно постоянно обращаться по полному пути структуры
PColors [dwIndex1].hls.wH такое постоянно, почему не сделать в начале что-то вроде
GPIXEL c = PColors [dwIndex1];
Ну и так далее.
Я плохо понял программу, но мне кажется SortSimilarHLS совсем лишня, ты же результат используешь для отображения в ImgSetupDlg
Тогда тебе достаточно информации о частоте появления цвета
Да и как-то уж медленно оно работает.
STL это стандартная библиотека — обязательно.
MFC идет гулять — Qt, для полноты картины, может быть Gtk. Что-нибудь для передачи сообщений...
MFC идет гулять — Qt, для полноты картины, может быть Gtk. Что-нибудь для передачи сообщений...
Кто еще добавит?
eugene_n, планирую просмотреть MFC, STL поглубже. Дальше какую-то библиотеку по современнее, наверное. Основные алгоритмы тоже просмотреть.
eugene_n, имеете ввиду библиотеки?
Можно и так сказать. Во всяком случае побольше шаблонных решений в копилку.
Вполне нормально, только стоит сдобрить это знаниями каких-нибудь фреймворков.
Mike Gorchak
мне главное ответить на вопрос, могу ли я искать работу с таким кодом.
Да.
Аноним, понятное дело, но никто ведь не запрещает узнать мнение. Мне оно интересно и важно.
???мне главное ответить на вопрос, могу ли я искать работу с таким кодом
Тоесть, если кто-то здесь скажет нет, вы засядете в самообучении еще на годик? Пытаться всегда нужно, и для этого не нужно спрашивать на форумах разрешение.
Аноним, мне главное ответить на вопрос, могу ли я искать работу с таким кодом. Про рост я знаю, всегда есть куда расти.
Что такое «джуниор WinSDK»?
Андрей
Человеку эти идентификаторы нагенерила студия
Студия пишет в комментах: Никогда неизменяйте эту программу, она сгенерирована студией!
Эта граница слишком размыта. Есть куда расти, и это главное.
Я же говорил, надо из модулей сделать классы (инкапсулировать). Тогда это будет С++ джуниор WinSDK.
Спасибо еще раз за советы по коду. Так канает он на позицию джуниора C++ или нет? Или может чуточку больше/меньше?
В данном случае — не надо.Человеку эти идентификаторы нагенерила студия, зачем это править?:)
А вот ручками так писать не стоит, хотя и не является смертным грехом.
Майк! Мы говорим о:
а) С++
Ну в общем-то да.
б) произвольно задаваемом списке идентификаторов.
Человеку эти идентификаторы нагенерила студия, зачем это править?:)
Майк! Мы говорим о:
а) С++
а) С++
б) произвольно задаваемом списке идентификаторов.
И вообще, использовать #define разрешено только фирме Майкрософт.Бред. Везде, где есть микс С и С++, const int использовать нельзя.
С энумом та же песня, тип enum’а может задаваться из вне, либо он жёстко равен int’у, а в случае с #define, можно типизировать все константные значения: U, L, UL, LL, ULL, f, e.
2 eugene_n
Привет.
Привет.
Ещё лучше для гарантии уникальности использовать объект, гарантирующий уникальность
Если переменная в модуле класса упоминается чаще, чем в других местах, значит это первый кандидат стать полем класса, для функций — не так, Если функция чаще упоминается в модуле, чем в других местах — это функция объявляется private, иначе — public. Вызов private-функции в других местах надо заменить на вызов public — функции, иначе не скомпилируется.
Читайте учебник Страустрапа по С++, лучшего пока не видел.
Ещё один кандидат на поле класса
extern HWND hWndDlg;Более 30 раз упоминается в файле ActionHndl.cpp
. Замените все
#define IDC_STATIC_TXT1 1001
на
const int IDC_STATIC_TXT1 = 1001;Это избавит Вас от головной боли при поиске ошибок типа
Я б предложил
enum {
IDC_STATIC_TXT1 = 1001;
...Это как бы гарантирует непересечениие идентификаторов.
И вообще, использовать #define разрешено только фирме Майкрософт.
Правильней сказать «использовать #define не для управления процессом компиляции».
Кстати, переменные, которые желательно сделать полями класса у Вас уже есть в файле ActionHndl.cpp, например:
extern char szFile[128];В данном файле он упоминается пять раз! Значит он первый кандидат стать полем класса. В других файлах у Вас есть близнец переменной,
char * ExtractFileName(char * szFile)
избегайте таких близнецов.
Моё мнение, что венгерская нотация не несёт смысловой нагрузки. ТОесть имена типа szFile ниочём не говорят. Главное не тип, а смысл, зачем эта переменная, что в ней записано. Префикс sz, передающий информацию о типе, нужен не строготипизируемым языкам, к которым плюсы не относятся, тут, если что, Вас компилятор поймает при ошибке типа.Зачем Вам функция DataAlloc, если проще и безопаснее использовать new ? Безопаснее, потому что результат функции DataAlloc Вы приводите явно к нужному типу, случайно скопировав, долго будете искать ошибку:
PColors = (PIXELNFO *)DataAlloc(hWndDlg, lpBmpNfo->lImgPixelCount * sizeof(PIXELNFO));
Сравните
PColors = new PIXELNFO[lpBmpNfo->lImgPixelCount];
проверяйте результат на неравенство NULL.
Начнём сначала.
1. Замените все
#define IDC_STATIC_TXT1 1001на
const int IDC_STATIC_TXT1 = 1001;
Это избавит Вас от головной боли при поиске ошибок типа
void f1(char qq);
//////////////////
// место лажи - вызов f1
/////////////////
f1(IDC_STATIC_TXT1);
Внимание, вопрос, чему равен аргумент qq (компилятор ошибки не даст) ?
И вообще, использовать #define разрешено только фирме Майкрософт.
2.
К какому классу относятся, например, функции из файла ActionHndl.h ?
Правильно, ни к какому. Значит через год Вы сами это забудете, и напишете функцию OpenImage в другом модуле, а компилятор скажет,что у Вас такая функция уже есть.Оберните все функции в класс, например
class MyEditImage {
void OpenImage(void);
void CloseImage(void);
void ImgSetup(void);
bool StartImageAdjustment(void);
// дальше остальные функции
.....
};
Неважно, что нет полей, только функции, поля могут появиться позже, по мере разработки
Нет конечно же, все зависит от логики ветвления...
по причине РАРа тож не смогу на код взглянуть... так что аноним прав, а с Зипом и Вин ОСи тоже дружат
Mike Gorchak,
Спасибо за разъяснения. Стилистика-эргономика, вобщем.
Спасибо за разъяснения. Стилистика-эргономика, вобщем.
Насчитал 5 new и 3 delete. Они должны быть всегда в равных количествах?
В коде перемешаны табы и пробелы, отчего в других редакторах необходимо подбирать размер таба.А в чем безбашенность, можно конкретнее (то, что она там есть, я знаю:), но хотелось бы знать в чем качественно выражается).
Стиль:
if (....)
{
}
тупо расходует свободное место в каждой из строк, так как происходит двойной indent. Несколько раз при пролистывании видел индент совсем потерявший структуру.
Советую прочитать и принять на вооружение вот это:
geosoft.no/…t/cppstyle.html
Ну и посмотреть вот это (многие правила спорны, но всегда видны десятки тысяч человеко-часов, которые стоят за каждым из правил):
Анонимус,
Ставить доп софт чтобы посмотреть поделку какого то студента я совершенно не собираюсь.Грубить совсем необязательно, не нравится — не смотри.
Сергей Волошин,
Учебная программа для редактирования растра. Разбивает на группы цвета в формате HLS. В группах одинаковые S-составляющие, что дает цвет от черного, до, например, яркозеленого. Меняет местами H и L для каждого пиксела в любых двух группах. Полоски — это группы, можно выделить несколько там и там, потом нажать кнопку со стрелкой. Изменения отображаются на исходной картинке. Для простоты можно выбирать картинки из папки «Special».
Просто просьба оценить качество кода как для ищущего работу джуниора. Ясное дело, что на что-то приличное оно не тянет.
по-моему, тут советуют не просить
решить задачу из лабы
, а приведенное топикстартером творение весьма на нее смахивает:))
На фоне того что есть платформонезависимый зип пользоваться раром это убожество
сильное утверждение, линуксоид детектед
> Анонимус, посмотрите и ругнитесь плз.
Я не пользуюсь винраром. Ставить доп софт чтобы посмотреть поделку какого то студента я совершенно не собираюсь.
На фоне того что есть платформонезависимый зип пользоваться раром это убожество.
Я не пользуюсь винраром. Ставить доп софт чтобы посмотреть поделку какого то студента я совершенно не собираюсь.
На фоне того что есть платформонезависимый зип пользоваться раром это убожество.
Кстати, ВинРар у тебя лицензионный и адресок свой не подкинешь чтобы соответствующие органы смогли проверить чистоту и легальность всего установленного софта?
Sergii Voloshyn
Ambianx,
а что делает програма?
а что делает програма?
Я попробовал открыть в ней 1 небольшую картинку (открывает только bmp), и потом что-то с ней очень долго делалось (в процентах с прогрессом наверное не стояло выводить 3 знака после запятой) и вывелись какие-то полоски.
Mike Gorchak,
А в чем безбашенность, можно конкретнее (то, что она там есть, я знаю:), но хотелось бы знать в чем качественно выражается).
Зануда,
Там вроде все удаляется, что берется, если я ничего не напутал. Операторами new начинаются списки.
А в чем безбашенность, можно конкретнее (то, что она там есть, я знаю:), но хотелось бы знать в чем качественно выражается).
Зануда,
Там вроде все удаляется, что берется, если я ничего не напутал. Операторами new начинаются списки.
Спасибо, Киянуш, снова delete..., сейчас буду пересматривать код. Насчет того что делает, идея такая: цвет в формате HLS содержит S, которое означает яркость для одних и тех же H, L. Это в программе называется «группой похожих HLS». Смысл в том, чтобы редактировать изображение путем перестановок H, L у разных пар групп. Например, если менять красный с зеленым, то получится красный с S зеленого и наоборот.
За один только ВинРар архив нужно бить по голове кирзовыми сапогами, код даже смотреть страшно.
Александр Колосов
Як на мене нормальне форматування (відкривав у MSVS 2010). Присутні магічні числа... І іноді дійсно немає delete.
Де ви діли delete, шановний?
Де ви діли delete, шановний?
А так нормально. Тільки незрозуміло шо воно робить з малюнком, та яка з нього користь
1. Безбашенное форматирование кода.
2. Вызов PeekMessage () для каждого пикселя, нужно вызывать для группы пикселей (i.e. 1024), иначе это будет perfomance killer.
2. Вызов PeekMessage () для каждого пикселя, нужно вызывать для группы пикселей (i.e. 1024), иначе это будет perfomance killer.
Из-за пункта 1, я просто пробежался по коду и это всё, что бросилось в глаза.
41 коментар
Додати коментар Підписатись на коментаріВідписатись від коментарів