Skip to content

add method setInModel#165

Closed
stev wants to merge 1 commit into
onPHP:masterfrom
stev:setInModel
Closed

add method setInModel#165
stev wants to merge 1 commit into
onPHP:masterfrom
stev:setInModel

Conversation

@stev

@stev stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor
// ...
return ModelAndView::create()->
    setInModel('title', $this->getTitle())->
    setInModel('date', $this->getDate())->
    setInModel('author', $person)->
    setView('path/to/tpl');

VS

// ...
return ModelAndView::create()->
   setModel(
      Model::create()->
         set('title', $this->getTitle())->
         set('date', $this->getDate())->
         set('author', $person)
    )->
    setView('path/to/tpl');

очень приквыкли к этому методу, воможно и другим пригодится

@dovg

dovg commented Dec 25, 2012

Copy link
Copy Markdown
Member

Инкапсуляция нарушается, не?

Что мешает сделать так:

return ModelAndView::create()->
   setModel(
      Model::create()->
      set('title', $this->getTitle())->
      set('date', $this->getDate())->
      set('author', $person)
    )->
    setView('path/to/tpl');

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

@dovg можно, но не трушно создавть (выделять память и команды VM) еще один объект модели когда он уже создан

К томуже, не всегда удобно, перезатирать имещуюся модель с данными.
Содавать отдельно объект Model для двух параметров и потом мержить - тоже не "камильфо"

Инкапсуляция модели? - когда она доступна и так, в рамках этого класса.
https://github.com/onPHP/onphp-framework/blob/master/main/Flow/ModelAndView.class.php#L37

Инкапсуляция начнет действовать только при переопределении в наследуемом классе. Не че не мешяет тебе переопределить методы.

@ewgRa

ewgRa commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

имхо плохая практика

зачем так сжимать код? Если так дальше рассуждать, то только для того чтобы делать короткую return конструкцию можно все методы model перетаскивать в ModelAndView. Это точно не очень хорошо.

@dovg

dovg commented Dec 25, 2012

Copy link
Copy Markdown
Member

Инкапсуляция начнет действовать только при переопределении в наследуемом классе.

лол.

Сообществу виднее, мы же в демократию играем. Свое мнение я выразил.

@AlexeyDsov

Copy link
Copy Markdown
Member

Так если не трушно выделять память на лишнюю модель - может сделать что б там модель не создавалась в методе __construct? А создавалась только по требованию. Что-нить вроде:

public function getModel()
{
    if (!$this->model) {
        $this->model = Model::create();
    }
    return $this->model;
}

Может даже такой pull request и забацаю :)

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

зачем так сжимать код?

@ewgRa встречный вопрос: зачем так плодить код ?

вот перетаскивать все методы - согласен, не хорошо.
Но код пишут для того чтоб было удобно и проксирование это обычная практика, не ..?

@AlexeyDsov это конечно классно... при запросе getModel() получить не Модель а MоdelAndView! - забацай! ;)

@AlexeyDsov и все равно это не то.

@AlexeyDsov

Copy link
Copy Markdown
Member

@stev поправился, забацаю. Есть еще к чем придраться?

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

Замечу, что Придераюсь не я

@pupkinV

pupkinV commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

Тут немного "зависти" у MAV к сигнатуре методов Model, но удобства это добавит, я гарантирую это!
@dovg трудно возразить, инкапсуляция (ну совсем чучуть) нарушается. Но вероятность что в mav появится другая модель с другими методами и их параметрами :/

Что мешает сделать так:

ты ведь сам прекрасно знаешь, что модель наполняется не в одном месте в коде.
#165 (comment) -- а это, по моему, нужно сделать независимо от того примут этот пул реквест или нет =)

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

@dovg хорошо заметил в "Демократию играем"

@pupkinV

pupkinV commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

вот перетаскивать все методы - согласен, не хорошо.

это ты форме скажи =) повеселил. она даже #101 (comment) "изКожиВон" инстансы проверяет перед тем как вызвать методы у содержимого примитива.

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

@pupkinV я про форму уже и не говорю, смех до абсурда

@ewgRa

ewgRa commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

@stev

встречный вопрос: зачем так плодить код ?

я не вижу ничего лишнего в том коде, который существует сейчас. Ты же не собираешься каждый раз когда тебе надо обратиться к двум объектам в пределах одной цепочки вызова создавать "короткий" метод?

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

@ewgRa Естественно не собираюсь, я понимаю о чем ты хочешь сказать, но если посмотреть на сущность ModelAndView - и на то что она не меняется (с лохматой 0.4 версии), тк она проста и самодостаточна.

Исключительно для удобства. В любом другом случае ситуация будет рассматриваться по-другому.

@ewgRa

ewgRa commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

и на то что она не меняется (с лохматой 0.4 версии), тк она проста и самодостаточна.

золотые слова. но тебе захотелось вдруг поменять.

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

@anisimovt

Copy link
Copy Markdown
Contributor

У кого-то оплата за строчки что ли? С чего бы ModelAndView брать на себя сеттеры модели?

    $mav->setModel(
         $mav->getModel()->set('foo', 'bar')
    )

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

    return $superObject->setInMavSetInModelSetInUser($foo); 

Такая редкая ситуация что ли? $object->getSubObject()->setProperty(), каждому объекту сверху сеттер наворачивать?

@pupkinV

pupkinV commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

создайте свой форк

здесь все взрослые и знают, что им делать. Давайте без этого. @anisimovt Вы, как и все "голосовавшие" против такого метода, я не вижу его сверх приоритетным, но приветствую. Думаю @stev в своем форке это уже сделал, но предлагает пользоваться всем и в этом мало противоестественного.

@anisimovt > Можно даже сделать почитайте. А ведь Form такой-же своеобразный объект использующийся исключительно для наследников BasePrimitive

@anisimovt

Copy link
Copy Markdown
Contributor

Может добавим тогда еще dropInModel? А то тоже цепочку не составишь. И да, я буду ёрничать, потому что считаю что коэфициент полезности реквеста стремится к нулю, а обсуждение уже на несколько экранов.

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

@ewgRa я тебя услышал

больше не вижу смысла продолжать беседу по этому топику.

@dovg

dovg commented Dec 25, 2012

Copy link
Copy Markdown
Member

Жаль, что тут нельзя ставить лайки к комментариям как в ФБ. ;)
#165 (comment) - предложение dropInModel выше всяких похвал!

@anisimovt

Copy link
Copy Markdown
Contributor

И кстати, какое-то странное понимание демократии. У нас вроде у кого-то есть расширенные права на этот репозиторий? Давайте всё будет почеловечески и ментейнер(ы) будет(ут) проводит модерацию пулреквестов.

@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

#165 - предложение dropInModel выше всяких похвал!

@dovg можешь к себе влить этот метод

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

что касается последнего @anisimovt поясни, что иммено подразумевается "Давайте всё будет почеловечески"

@anisimovt

Copy link
Copy Markdown
Contributor

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

@AlexeyDsov AlexeyDsov closed this Dec 25, 2012
@stev

stev commented Dec 25, 2012

Copy link
Copy Markdown
Contributor Author

@pupkinV

pupkinV commented Dec 25, 2012

Copy link
Copy Markdown
Contributor

@stev да, ты немного не поспел с этим аргументом, а он весОм! Коэффициент непредвзятости оставляет желать...
правда там структура несколько иная. Наподобие нашей Form, где key => prm хранятся прямо в коллекции.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants