Помогите исправить собственное быдлокодерство

[WMix]

код настолько запутанный что автор не может понять почему он получил такой результат, а другие программисты не могут понять что же хотел автор.

это понятно, но всеже мне кажется это произошло от того, что автор не умеет писать на php и не знает инструментов которые там имеются для реализации той или иной проблемы... проектировать, не зная что правильнее былоб в той или иной ситуации использовать, кажется мне глупым!

 

[WMix]

craz
мне легче читается конструкции учитывая ответ

$return = mysql_num_rows($result);

return ( $return > 0 ? $return : false );

а //тут что-то произошло чего я совсем не ожидал надо как-то меня об это предупредить, вероятнее всего решается try{}catch(){} конструкцией

 

[craz]

craz

а //тут что-то произошло чего я совсем не ожидал надо как-то меня об это предупредить, вероятнее всего решается try{}catch(){} конструкцией

Это понятно, причем try тут уже не причем) ты наверное про throw? Я же не имел ввиду что там останется комментарий в продакшене)

Остальное субъективно я по-этому и написал ИМХО.

 

[Фанат]

с самого начала принят неправильный подход к решению задачи

Всё верно. Для случая, когда этот подход вообще есть. Когда программист понимает смысл своего кода.
Здесь же ничего этого и в помине нет - есть механическое копирование кусков кода.

Это принципиальная разница.
Бесполезно заставлять программиста отлаживать свой код, если он не знает, как этот код работает. поскольку само понятие отладки подразумевает, что программист знает, какое значение должно быть в каждой переменной в любой момент времени - и в процессе отладки сравнивает эти значения с фактическими.

С рефакторингом то же самое.
Если программист не понимает, что делает его код, то и отрефакторить его не сможет.

 

[Фанат]

я боюсь таких вещей когда вижу их в коде.

таких вещей бояться не надо.
Надо понимать, что оператор сравнения УЖЕ возвращает булево значение, и не писать в коде мало масляное.

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

$sql = "SELECT 1 FROM users u, groups g WHERE u.id=?i AND sysName='manager' AND group_id=g.id";
$isManager[$id] = $db->getOne($sql,$id);

 

[craz]

таких вещей бояться не надо.
Надо понимать, что оператор сравнения УЖЕ возвращает булево значение, и не писать в коде мало масляное.

Но не стоит это прям в логику инъектить, но возвратил и возвратил, давай другой пример.

$return = myOtherFunction($m);

в его случае return (myOtherFunction($m)==1)

Мне вот страшно, что там не булевое значение, что в первом(это просто тупо не понятно), что во втором случае(а с чем вообще у меня 1, то сравнивается с каким типом хотя бы), я хочу это максимально контролировать.

$isManager[$id] = $db->getOne("SELECT 1 from users WHERE group_id=100 AND id=?i",$id);

"SELECT 1"? Я такого не делаю. Кстати без шуток, а зачем в массив может понадобиться добавлять по одному значению? почему не fetchAll?
Ну то есть я бы понял если бы было

$Registry::isManager()->set($id)=$db->getOne("SELECT 1 from users WHERE group_id=100 AND id=?i",$id);

Но просто в коде я бы встретил и опять испугался бы..


P.s. Блин чето я на измене седня чтоль

 

[craz]

Короче ладно, я тебя понял. Бояться не надо: В Офигенном таком продукте тобой написанном. Я бы не испугался.

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

P.s.
Ну и наверное последние, что здесь напишу. (Не хочу просто тему портить своим мнением, оно реально субъективно). Я наверняка тоже быдлокодер. Но я когда вижу те или иные подходы в решении задач, они мне субъективно нравятся или нет. Но вот такого быдлокода который вы начали обсуждать еще на 1 странице, его чуть больше, чем весь существующий код.

 

[Фанат]

давай другой пример.

Тебя не в первый раз уже уносит в сторону.
Другой пример будет в другом топике.
А здесь свои примеры. И конкретно в этом топике можно возвращать сразу num_rows, а в массив нужно возвращать по одному значению.

 

[gray]

У вас ввиду моего молчания наверное сложилось, на мой взгляд, забавное впечатление. Попробую немного прояснить ситуацию:
1. программный продукт уже действует. Писал неумело, как владел инструментом. Но он работает и приносит деньги. Использовал вначале блокнотоподобный pspad, затем потратил время на освоение eclipse. Сейчас это, по моим меркам ( т.к. не разрабатывал программы в течении нескольких месяцев), это уже увесистое ПО. Не очень хорошее, но я над этим работаю - дописываю и расширю идею.
2. На форуме я оказался из-за: а. желания узнать что-то новое; б. я знаю что пишу как неандерталец на php, и хочу это исправить, а следовательно попробовать получить мнение по тому как будет правильно описывать задачи.
3. К php я вернулся в 2012 году. Оставил программирование в 2007. Сейчас возникла необходимость, потому и программирую вновь.
4. Знания о php и подходах в программировании мои не велики ( о чем я пишу и что можно видеть по моим кускам кода). И во многом я действительно открывал для себя много нового как в php так и в других областях: javascript,ajax, jquery,bootstrap etc.
5. Когда я осваивал php никакого PDO в обиходе не было (или я к нему тогда не успел прийти), потому пользуюсь mysql_query (книги мои по php датированы до 2007-го года и там других способов подключения к БД не было). Понимаю что это каменный век, не безопасно и т.п. За pdo я обязательно сяду и буду разбираться в нем, как и во многом другом что я не знаю.

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

$sql = "SELECT 1 FROM users u, groups g WHERE u.id=?i AND sysName='manager' AND group_id=g.id";
$isManager[$id] = $db->getOne($sql,$id);

Идея - класс. Получается можно проверять как одного пользователя, так и всех пользователей вообще. Интересно.
Сделаю аналогично, но пока без pdo. За идею спасибо.

 

[weregod]

а если говорить о конкретно коде, то достаточно и

$return = mysql_num_rows($result);

если говнокодить на голом mysql

если я ожидаю булево на выходе, то либо

$return = mysql_num_rows($result) > 0;

либо

$return = (bool)mysql_num_rows($result);

 

[gray]

Доброе утро.
Переделал ф-ю. Вот что получилось:

function isAdmin($id){
		static $truefalse;
		if (!isset($truefalse[$id])) {
			$query="SELECT 1 FROM `users` `t1`, `groups` `t2` WHERE `t1`.`id`='".mysql_real_escape_string($id)."' AND `t2`.`sysName`='admin' AND `t1`.`group_id`=`t2`.`id`;";
			$result=mysql_query($query) or die ("Ошибка выполнения запроса к БД: ".mysql_error());
			$truefalse[$id]=(bool)mysql_num_rows($result);
		}
		return $truefalse[$id];
}

Хотелось бы узнать ваше мнение.

 

[WMix]

я поначалу глупость написал, стер...
$id это индификатор пользователя... для каждого пользователя (для каждого вызова странички) $truefalse создается по новой... на каждый вызов isAdmin($id) дергается база... (не совсем правда) !!!
много ли мы выйграем обьявив переменную static? нужен ли масив? возможно ли static $truefalse сразу заполнить (одной выборкой из базы)? вдумайся что находиться в переменной $truefalse и дай ей более осмысленное имя...

 

[gray]

WMix
логично. если значение false у $truefalse[$id], то она уже была проверена. и в этом случае выведется просто значение false. проверил в действии, вроде бы именно так и работает. Ключ у массива есть, но он с пустым значением. Вот пример вывода, когда на вход подаются последовательно: 1,22,22,5,23,23,22. if - когда срабатывает if:

id=1 if Array ( [1] => ) 
id=1 Array ( [1] => ) 
id=22 if Array ( [1] => [22] => 1 ) 
id=22 Array ( [1] => [22] => 1 ) 
id=5 if Array ( [1] => [22] => 1 [5] => ) 
id=23 if Array ( [1] => [22] => 1 [5] => [23] => ) 
id=23 Array ( [1] => [22] => 1 [5] => [23] => ) 
id=22 Array ( [1] => [22] => 1 [5] => [23] => )

 

[craz]

$trueFalse - фееричное название.
http://msgeeks.ru/?artid=55 - читаем хотя бы это.

 

[gray]

я поначалу глупость написал, стер...
$id это индификатор пользователя... для каждого пользователя (для каждого вызова странички) $truefalse создается по новой... на каждый вызов isAdmin($id) дергается база... !!!
много ли мы выйграем обьявив переменную static? нужен ли масив? возможно ли static $truefalse сразу заполнить (одной выборкой из базы)? вдумайся что находиться в переменной $truefalse и дай ей более осмысленное имя...

На каждый вызов страницы - да, свой truefalse, но многократный вызов функции не создает новый truefalse. функция проверяет существование в ней уж имеющегося ключа $id и если его нет, то создает новый ключ с запросом к БД.

в основном у меня проводится проверка одного и того же идентификатора пользователя. но такой вариант, как правильно сказал Фанат, позволяет мне проверять нескольких пользователей.
изначально я писал тоже по принципу, забираем все значения идентификаторы из БД и ищем в выдаче $id. опять же, фанат подсказал что если у меня будет мегабольшое кол-во пользователей (мильён, например. что я точно знаю не будет, максимум 1000 и то через год-полтора), то у меня получится ситуация что я обрабатываю большой массив. с другой стороны, в будущем мне потребуется администраторам дать возможность обрабатывать группы пользователей какими либо функциями. а этот вариант позволяет это делать постепенно наращивая массив. т.е. если мне нужно будет обработаться только сотрудников, мне не нужно хранить информацию еще и клиентов, которых в разы больше.
вот как-то так.

 

[WMix]

ну если ты все можешь так хорошо обосновать, мне нравиться твой код... все сказанное верно!

 

[gray]

craz,WMix
Вариант $return, $usersId (не знаю почему не $users_id, просто претит мне использовать подчерк в наименовании, как советует Макконелл).

 

[craz]

craz,WMix
Вариант $return, $usersId (не знаю почему не $users_id, просто претит мне использовать подчерк в наименовании, как советует Макконелл).

не нравиться ни используй.
камелКейс нормальная альтернатива.
я честно говоря не понимаю правда твоего кода...

function isAdmin($id){ 
        if (!isset($truefalse[$id])) {
            $sql="SELECT 1 FROM `users` `t1`, `groups` `t2` WHERE `t1`.`id`='".mysql_real_escape_string($id)."' AND `t2`.`sysName`='admin' AND `t1`.`group_id`=`t2`.`id`;";
            $query=mysql_query($query) or die ("Ошибка выполнения запроса к БД: ".mysql_error());
            $result=(bool)mysql_num_rows($result);
        }
        return $result;
}

Еще раз мне объясните почему не так? Из-за мефического хайлоада?

Аааа я понял тут архитектура кривая, блин, а я то... isAdmin должен браться из сессии пользователя, а проверяться чуть ли не в бутстрапе.

Короче как всегда проблема свелась не к тому КАК ты пишешь, а ЧТО ты пишешь...

 

[Фанат]

На самом деле не обязательно применять один и тот же подход.
То есть, если делать унифицированно, то обязательно:

function checkRights($is,"admin")

- здесь код должен быть единообразным для всех вариантов. Но зато количество функций уменьшится.

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

Хотя на странице, где идёт 70 проверок, скорее всего просто неверно построен запрос к БД.

 

[WMix]

на самом деле напрашивается $isAdmin[$id]... но чесно это не столь важно, если вдуматься в топик выше, желание сделать функцию более широкой, то можно было бы написать так

function inGroup($user_id, $group='admin')

и в таком случае назвать переменную $usersGroup....

меня опередили ))