s_o_v_a 7 сен 2016 в 10:21

Плохой код убивает

10 мин

72K

Recovery Mode

Плохой программист Джон сделал ошибку в коде, из-за которой каждый пользователь программы был вынужден потратить в среднем 15 минут времени на поиск обхода возникшей проблемы. Пользователей было 10 миллионов. Всего впустую потрачено 150 миллионов минут = 2.5 миллиона часов. Если человек спит 8 часов в сутки, то на сознательную деятельность у него остается 16 часов. То есть Джон уничтожил 156250 человеко-дней ≈ 427.8 человеко-лет. Средний мужчина живет 64 года, значит Джон убил примерно 6 целых 68 сотых человека.

Как тебе спится, Джон — серийный программист?

Пользователи — низ пищевой цепочки. Программисты также медленно убивают друг друга.

Правила хорошего кода

Простота-понятность-компактность, производительность, отсутствие дублирования.

Если вы пишете что-то сложнее «hello world”, оно будет размещаться не в одном, а в нескольких файлах. Как правило, файлов бывает больше десятка. Всем им даются непонятные короткие имена (программисты любят сокращения) Человек, который попытается разобраться в вашем коде, будет плеваться, шипеть и проклинать вас. Ваша карма будет испорчена и следующие несколько жизней вы будете собакой в Корее.

Примечание: «hello world” — не ошибка, именно так поставил кавычки LibreOffice Writer. Джонни, привет! Всегда рад тебе! Шучу, никогда.

Как выбрать хорошее имя

Хорошее имя максимально короткое, но при этом максимально точно отражает суть объекта. Ваш код будут читать люди (и вы сами перед сном), поэтому имена должны быть понятны людям.

Джон_плохой_программист — хорошее имя. Хорошее имя состоит из одного, двух или трех слов. Три — много. Одно — хорошо, но часто не хватает чтобы отразить суть. Прочитайте следующие имена файлов (классов), можете ли вы угадать назначение кода, не заглядывая внутрь?

profiler.h
jitter_measurement.h
developer_menu.h
Animation2D.h
Rectangle.h
Wind_effect.h

Имя объекта внутри файла совпадает с названием файла. Путаница не нужна.

Если после моего объяснения вы продолжите давать файлам непонятные имена вроде SoMFVec2s.h, то застрелитесь. Это рациональный поступок, ведь тем самым программист спасает несколько чужих жизней.

Замечание: ограничение на имена файлов 8.3 снято более 20 лет назад.

Нужны ли именам префиксы?

НЕТ.

Визуальный шум не нужен.

Современные «текстовые редакторы для программистов» выдадут вам информацию о прототипе чего угодно по нажатию одной кнопки или наведению курсора мышки. Поэтому любые префиксы в 20NN году совершенно излишни.

То же самое можно сказать о пространствах имен — пространство имен — это префикс. Постоянно используя префиксы типа std:: cv:: вы конечно гарантируете себя от ошибки случайного использования объекта из другого пространства имен. НО!

Вы хоть раз так ошибались?
Приводила ли ошибка к чему-то серьезному?
А набивать лишние символы приходится сотни и сотни раз.
Оцените издержки. Это ваша единственная жизнь, между прочим.
Потратил 100 часов жизни на набивание префиксов? Ты нормальный? Лучше бы погулял сходил… Мама

Можно ли давать объектам короткие имена

НУЖНО.

int i;
char c;
byte b;
string s;
vector v;
pointer p;
argument a1;

Короткие имена прекрасны, так как содержат минимум визуального шума, читаются и понимаются быстрее всего. Часто совершенно ясно над чем вы производите действие, поэтому обязательно дайте этому объекту короткое имя для удобства и чукчи-писателя и чукчи-читателя. В пределах области видимости пол-экрана — экран, короткое имя будет благом для всех.

Подпространство

В окне «Панель управления windows», вы видите объекты «Установка обновлений windows», «Сервисы windows» и «Шрифты windows». Несомненно вас считают за идиота, который забыл где находится. А также за человека, который любит нагружать свой мозг: раз за разом искать нужный объект, отфильтровывая визуальный шум.

В подпространстве находится 52 объекта, не разделенных на группы ни по какому признаку. Костыля: «быстрый поиск по паре букв с клавиатуры» также нет. Наслаждайся.

Используйте подпространство с умом. Имя библиотеки, имя директории, имя файла, пространство имен, название класса, название функции. К моменту, когда читатель смотрит на вашу функцию, он уже прошел 5 уровней имен. Программисты уж никак не умственно отсталые, они помнят контекст.

Если вы пишете понятный код с разумными именами, то комментировать вам практически нечего. Только места с неочевидными вещами, хаки. Лучше потратьте энергию на короткое описание что же все таки делает этот модуль.

Кстати...

…. а давайте создадим вместе с нашей программой сотню других, совершенно бесполезных, которыми никто не будет пользоваться. И будем заставлять пользователя устанавливать на диск их все. Не дадим ему выбора. А свою основную работу выполним кое как — тяп ляп и готово. И так сойдет! Хахаха!

Какая сложная, объемная, нестабильная и склонная к саморазрушению у нас получилась система! Потрясающе!

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

А давайте создадим в нашей операционной системе кучу сетевых сервисов, которыми никто не будет пользоваться. Но мы все их включим по умолчанию. В наших сервисах будет множество уязвимостей, желательно уровня remote code execution и мы будем постоянно их исправлять! Кайф! А еще наш установщик обновлений будет зависать потребляя гигабайт памяти и 100% ресурсов процессора.

И мы будем делать ошибки. Пусть например наше обновление KB3136000 будет устанавливаться много раз.

Джон, ты гений!

Сложно!

На поиск этой функции я потратил минут 20. Простая задача: разбить строку по разделителям. Простая задача должна решаться максимально просто:

void split(const string& s, std::vector<string>& result, const string& delimiters = " ")
{
	string::size_type a = s.find_first_not_of(delimiters, 0);
	string::size_type b = s.find_first_of(delimiters, a);

	while (string::npos != b || string::npos != a)
	{
		result.push_back(s.substr(a, b - a));

		a = s.find_first_not_of(delimiters, b);
		b = s.find_first_of(delimiters, a);
	}
}

А затем я упростил эту функцию, без ущерба ликвидировав дублирование:

void split(const string& s, std::vector<string>& result, const string& delimiters = " ")
{
	string::size_type a, b = 0;

	for(;;)
	{	a = s.find_first_not_of(delimiters, b);
		b = s.find_first_of(delimiters, a);

		if (string::npos == b && string::npos == a) break;

		result.push_back(s.substr(a, b - a));
	}
}

Если вы можете решить задачу проще — сделайте это и я сочиню небольшую хвалебную оду в вашу честь. В интернете множество людей предлагают использовать для разбиения строки: boost, регулярные выражения, черта из ада, космический корабль пришельцев… Возьми в проект огромный boost, занимающий несколько гигабайт места на SSD диске, вместо маленькой функции… «Ну ведь оно потом ищо для чего-нибудь пригодится...»

Дорогой друг, мне всего навсего разбить строку! Буст? Это как если бы тебе нужно было пришить пуговицу, а люди предлагают здоровенный портняжный китайский агрегат в несколько тонн весом. Он умеет пришивать пуговицы. И несомненно еще для чего-нибудь пригодиться. Например, он сможет подшить тебе твой правый ботинок через полгода, когда тот порвется. Но издержки…

Кто виноват, что Джонни не позаботился о наличии в могучем языке C++ стандартного способа выполнить стандартное действие? Джон, скольких программистов ты уже убил?

О скорости кода

Джонни, который создает «профилировщики производительности» в средах программирования — ты самый отвратительный убийца. Твой код не делает ничего полезного уже много лет. Люди же тратят мегачасы времени, пытаясь выдоить из твоей животинки хоть немного смысла, дергая ее за разные места…

В конструкции вроде этой, больше красоты и мощи, чем во всех твоих творениях:

unsigned long t0 = current_time();
// какой-то код

cout << current_time() - t0 << endl;

Ниже мой профилировщик, который кочует из проекта в проект уже 10 лет, слегка мутируя под особенности операционных систем и способы вывода информации на экран.

profiler.h

/*
Profiler prof;

for (;;)
{
	Sleep(50); // code, which does not need to measure performance

	prof(NULL);

	Sleep(100); // some code

	prof("code1");

	Sleep(200); // some code

	prof("code2");

	prof.periodic_dump(5);
		// every 5 seconds will print table
}
*/

#include <stdint.h>
#include <stdio.h>

#include <string>
using std::string;

#include <set>
using std::set;

#include <algorithm>
using std::min;
using std::max;

#ifdef WIN32
#include <Windows.h>

class Microseconds
{
public:
	uint64_t operator()()
	{
		LARGE_INTEGER now;
		QueryPerformanceCounter(&now);
		LARGE_INTEGER freq;
		QueryPerformanceFrequency(&freq);

		return now.QuadPart * 1000 / (freq.QuadPart / 1000);
			// overflow occurs much later
	}
};

#else
#include <sys/time.h>
//#include "android_workarround.h"
class Microseconds
{
public:
	uint64_t operator()()
	{
		timeval tv;
		gettimeofday(&tv, NULL);
		return (uint64_t)tv.tv_sec * 1000000 + tv.tv_usec;
	}
};
#endif

class Profiler
{
	Microseconds microseconds;

	class Event
	{
		public:
		const char* name;
		uint64_t time;
		uint64_t count;
		uint64_t min_time;
		uint64_t max_time;

		void reset()
		{
			time = 0;
			count = 0;
			min_time = (uint64_t)-1;
			max_time = 0;
		}
	};

	class Comparator
	{
		public:
		bool operator()(const Event& a, const Event& b) const
		{	//return strcmp(a.name, b.name) < 0;
			return (void*)a.name < (void*)b.name;
		}
	};

	set<Event, Comparator> events;

	uint64_t t0;
	uint64_t last_dump;

	Event c;
	set<Event>::iterator i;

public:
	Profiler()
	{
		last_dump = t0 = microseconds();
	}

	void operator()(const char* what)
	{
		if (what == NULL)
		{
			t0 = microseconds();
			return;
		}

		uint64_t t = microseconds() - t0;

		c.name = what;
		i = events.find(c);

		if (i == events.end())
		{
			c.reset();
			i = events.insert(c).first;
		}

		Event& e = const_cast<Event&>(*i);

		e.time += t;
		e.min_time = min(e.min_time, t);
		e.max_time = max(e.max_time, t);
		++e.count;

		t0 = microseconds();
	}

	void dump()
	{
		const float MS = 0.001f;

		float f_summ = 0;
		for (i = events.begin(); i != events.end(); ++i)
			f_summ += (float)i->time;

		if (f_summ == 0) return;

		f_summ *= MS;
		f_summ *= .01f; // %

		printf("           name count   total(%%)        min   avg   max\n");

		for (i = events.begin(); i != events.end(); ++i)
		{
			Event& e = const_cast<Event&>(*i);

			if (e.count == 0) e.min_time = 0;

			float f_time = e.time * MS;
			float f_min = e.min_time * MS;
			float f_max = e.max_time * MS;
			float f_average = e.count == 0 ? 0 : f_time / (float)e.count;

			printf("%15s %5llu %7.1f(%5.1f%%) %5.1f %5.1f %5.1f\n",
				e.name, (long long unsigned int)e.count,
				f_time, f_time / f_summ, f_min, f_average, f_max);

			e.reset();
		}
	}

	void periodic_dump(unsigned int period)
	{
		if (microseconds() < last_dump + period * 1000000) return;
		dump();
		last_dump = microseconds();
	}
};

В консоли вывод выглядит так, все время в миллисекундах:

           name count   total(%)        min   avg   max 
       detector     0     0.0(  0.0%)   0.0   0.0   0.0 
      predictor   161   287.8( 46.1%)   1.0   1.8   2.3 
        refiner   161   246.9( 39.5%)   0.8   1.5   1.8 
      shape fit   161    90.0( 14.4%)   0.3   0.6   0.8

Обратите внимание, вверху в комментарии к исходному коду дан работающий пример.

Если вдруг нужно вывести таблицу на экран, то dump модифицируется так, чтобы записывать свой вывод в vector out; Далее графическая часть отрисовывает текст моноширинным шрифтом. Вывод профайлера у вас на экране. Оценить скорость старой и новой функции одновременно, в живом приложении? Пожалуйста. Только не забывайте про кэши процессора.

Предложения по упрощению-улучшению профилировщика принимаются. Простая задача — простое решение. Джонни, сделай мне приложение на 500 гигабайт, делающее то же самое, но хуже.

Что такое установка программы

Некие заскриптованные предварительные действия, создание «хорошей рабочей среды» для программы, в противовес «плохой среде», в которой программа запуститься не может. Признание программиста что, он криворук и делает допущения о состоянии системы при старте своей программы.

Ваша программа настолько плоха, что в работе ее приходится переинициализировать несколькими разными способами. Сломалось? Перезагрузи. Все равно не работает? Переустанови. Убийство времени пользователя — убийство пользователя.

Ты выложил код в интернет

Ты сделал это, молодец! Ты поделился продуктами желудочков твоего мозга с человечеством. Скольких ты убил? Сколько программистов умерло, пытаясь понять что делает этот файл? Сколько судеб ты стер из реальности, не проверив собирается ли твое творение на двух популярных операционных системах? Может стоило вместо длиннющего копирайта «Юниверситет им. Василия Пупкина, все права защищены», вставить хотя бы одну строку с описанием что же делает этот модуль? Ах код сложно было писать, пусть они помучаются… ну-ну… Корея. Гав.

Дорогой студент — не пиши код.
Если пишешь — не сохраняй.
Если сохраняешь — не выкладывай.
Если выложил — не в интернет.
Если в интернет — удали.
Если удалил — сделал добро людям.

Послесловие

Вышло новое бесполезное обновление windows, время установки 15 минут. Погибло 5048 человек…

Исправленный по критике хабровчан profiler.h

profiler.h

/*

#ifdef linux
#include <time.h>
void Sleep(int milliseconds)
{
	struct timespec ts;
	ts.tv_sec = milliseconds / 1000;
	ts.tv_nsec = (milliseconds % 1000) * 1000000;
	nanosleep(&ts, NULL);
}
#endif

int main()
{	Profiler prof;

	for(;;)
	{	
		Sleep(50); // code, which does not need to measure performance

		prof(NULL);

		Sleep(100); // some code

		prof("code1");

		Sleep(200); // some code

		prof("code2");

		prof.periodic_dump(5);
			// every 5 seconds will print table
	}
	return 0;
}
*/

#include <stdint.h>
#include <stdio.h>

#include <vector>
using std::vector;

#include <string>
using std::string;

#include <map>
using std::map;

#include <algorithm>
using std::min;
using std::max;

#include <utility>
using std::make_pair;

#include <iostream>
using std::cout;

#ifdef linux

#include <sys/time.h>

static uint64_t microseconds()
{	timeval tv;
	gettimeofday(&tv, NULL);
	return (uint64_t)tv.tv_sec * 1000000 + tv.tv_usec;
}

#else

#include <Windows.h>

static uint64_t microseconds()
{
	LARGE_INTEGER now;
	QueryPerformanceCounter(&now);
	LARGE_INTEGER freq;
	QueryPerformanceFrequency(&freq);
	return now.QuadPart * 1000 / (freq.QuadPart / 1000);
		// overflow occurs much later
}

#endif

class Profiler
{	
	class Event
	{
	public:
		uint64_t time;
		uint64_t count;
		uint64_t min_time;
		uint64_t max_time;

		void reset()
		{	time = 0;
			count = 0;
			min_time = (uint64_t)-1;
			max_time = 0;
		}
	};

	map<const char*, Event> events;

	uint64_t t0;
	uint64_t last_dump;

	map<const char*, Event>::iterator i;

public:
	vector<string> out;

	Profiler()
	{	last_dump = t0 = microseconds();
	}

	void operator()(const char* what)
	{	
		if (what == NULL)
		{
			t0 = microseconds();
			return;
		}
		
		uint64_t t = microseconds() - t0;

		i = events.find(what);
		
		if(i == events.end())
		{	
			Event e;
			e.reset();
			i = events.insert(make_pair(what, e)).first;
		}

		Event& e = (*i).second;

		e.time += t;
		e.min_time = min(e.min_time, t);
		e.max_time = max(e.max_time, t);
		++e.count;

		t0 = microseconds();
	}

	void dump()
	{
		out.clear();

		const float us_to_ms = 0.001f;
		
		float summ = 0;
		for (i = events.begin(); i != events.end(); ++i)
		{	
			Event& e = (*i).second;

			summ += (float)e.time;
		}

		if (summ == 0) return;

		summ *= us_to_ms;

		out.push_back("           name count   total(%)        min   avg   max\n");

		for(i = events.begin(); i != events.end(); ++i)
		{
			Event& e = (*i).second;

			if(e.count == 0) e.min_time = 0;

			float time = e.time * us_to_ms;
			float min_time = e.min_time * us_to_ms;
			float max_time = e.max_time * us_to_ms;
			float average = e.count == 0 ? 0 : time / (float)e.count;

			char tmp[0x100];

			snprintf(tmp, sizeof(tmp), "%15s %5llu %7.1f(%5.1f%%) %5.1f %5.1f %5.1f\n",
				i->first, (long long unsigned int)e.count,
				time, time / summ * 100, min_time, average, max_time);
			out.push_back(tmp);
			
			e.reset();
		}

		for (int i = 0; i < out.size(); ++i) cout << out[i];
	}

	void periodic_dump(unsigned int period)
	{	if(microseconds() < last_dump + period * 1000000) return;
		dump();
		last_dump = microseconds();
	}
};

Теги:

Хабы: