Это плохой стиль?
В итоге я написал немного кода:
Asteroid *a = new Asteroid( asteroidCollection ) ;
И оказалось, что мне не нужна переменная aпотому что в Asteroid конструктор new Asteroid закончил тем, что добавил себя к asteroidCollection,
Так что я смог написать:
new Asteroid( asteroidCollection ) ;
Это плохой стиль, учитывая, что мне даже не нужно возвращение? Должен ли я сделать это
asteroidCollection->createNew() ;
Или же
asteroidCollection->add( new Asteroid() ) ;
?
Решение
Я бы сказал, что это плохой стиль, потому что это сбивает с толку, когда вы читаете код. Первое, что приходит на ум, — это утечка памяти. Путаница испаряется только после того, как заглянуть внутрь конструктора и выяснить, что объект добавляет себя в некоторый список, который будет удален позже.
Также подумайте о возможных сценариях исключений. Было бы лучше сохранить выделенный объект в некотором интеллектуальном указателе, а затем добавить его в коллекцию.
Другие решения
На ваш вопрос я бы предпочел
asteroidCollection->add( new Asteroid() ) ;
чем два других стиля.
Это лучше, чем 1-й, потому чтоnew Asteroid( asteroidCollection ) ; не лучше чем то вроде
Foo( barCollection );
где Foo это класс, вы new объект Bar в его конструкторе и добавление его в коллекцию, слишком непонятно, чтобы знать, что происходит.
Это также лучше, чем 2-й, потому что ИМО asteroidCollection не должно волновать, как Создайте Asteroid, это коллекция, так что делать то, что коллекции делают.
Вопросы стиля всегда немного субъективны. Так что в некотором смысле мне нравится отвечать на них. Это мои 2 цента …
Просто подумайте на минуту, что вы хотите иметь возможность создавать объекты, полученные из Asteroid и добавьте их в свою коллекцию. Если add() случается, что метод вызывает любые функции Asteroid это могут быть (или использовать) виртуальные функции, вы открываете себе возможность доступа к объекту, который еще не был полностью построен.
Я думаю, это сбивает с толку — выполнять какие-то действия в конструкторе и оставлять их на усмотрение пользователя, чтобы понять, что происходит.
Я бы предпочел просто сделать простую функцию. Это даже не должно быть частью класса …
Asteroid *CreateAsteroid( AsteroidCollection *coll )
{
Asteroid * a = new Asteroid();
coll->add(a);
return a;
}
Если вы хотите остаться с подходом конструктора, вы можете, по крайней мере, вставить свою исходную строку кода в эту функцию и прокомментировать ее с очень четким описанием того, что происходит.
В основном, старайтесь не делать смешных вещей за кулисами. И если это неизбежно (или каким-то образом желательно), постарайтесь не заставлять кодера понимать странность, будь то кодер вы или кто-то еще в будущем.
По крайней мере, в этом случае это не совсем тот странно. Все могло быть хуже! знак равно
Просто чтобы взять ваши последние два примера …
-
asteroidCollection->createNew(); -
asteroidCollection->add( new Asteroid() );
Я думаю, что номер 1 это хорошо, если вы знаете, что это всегда один тип Asteroid создается, и вы не хотите, чтобы пользователь вашего класса беспокоился об этом. Если Asteroid не может быть разделен между коллекциями, имеет смысл сделать это. Это похоже на предложенную мной функцию, но в некотором роде класс коллекции предоставляет код поддержки, который, возможно, не должен.
Номер 2 полезен, если вы можете хранить производные Asteroid типы или если вы хотите сделать специальные вещи с астероидом, прежде чем добавить его в коллекцию. Это самый гибкий из всех трех подходов.
Это выглядит действительно проблематично, кто управляет временем жизни астероида?
Действительно ли он разделен между пользователем астероида и коллекцией, если это так, вы, вероятно, должны иметь shared_ptr для астероида, чтобы последний, кто имеет его, очистил его.
Если невозможно создать астероид без его помещения в коллекцию астероидов, то коллекции принадлежит время жизни.
Что произойдет, если вы создадите астероид в стеке? Что произойдет, если я добавлю этот астероид в unique_ptr или shared_ptr вместо прямого вызова new, например:
auto asteroid = std::make_shared(asteroidCollection)
он будет удален дважды?
Если вы не хотите, чтобы кто-то что-либо делал с астероидом, тогда, вероятно, более уместен метод на коллекции астероидов, createAsteroid или addAsteroid. Простой вызов метода createNew звучит так, как будто вы создаете новую коллекцию AsteroidCollection, которая, я думаю, не соответствует вашим ожиданиям.
detector