Замечания по программе Bodies

[alexey-malov]

Пример с виртуальным деструктором, рассмотренный на встрече:

// virtual_destructor.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include <memory>

using namespace std;

class CInternalObject
{
public:
    ~CInternalObject()
    {
        cout << "~CInternalObject" << endl;
    }

};

class CBase
{
public:
    virtual ~CBase()
    {
        DoSomething();
    }

    virtual void AbstractMethod() = 0;

    virtual void DoSomething()
    {
        //AbstractMethod();
        cout << "CBase::DoSomething()" << endl;
    }
};

class CDerived : public CBase
{
    CInternalObject m_someObj;
    virtual void DoSomething()override
    {
        cout << "CDerived::DoSomething()" << endl;
    }

    virtual void AbstractMethod() override
    {
        cout << "CDerived::AbstractMethod" << endl;
    }

};

int _tmain(int argc, _TCHAR* argv[])
{
    {
        cout << "automatic lifetime" << endl;
        CDerived derived;
    }

    {
        cout << "dynamic lifetime 1" << endl;
        auto derived = new CDerived();
        delete derived;
    }

    {
        cout << "dynamic lifetime 2" << endl;
        auto derived = make_unique<CDerived>();
    }

    {
        cout << "dynamic lifetime 3!!!" << endl;
        unique_ptr<CBase> base = make_unique<CDerived>();
    }

    {
        cout << "dynamic lifetime 4" << endl;
        shared_ptr<CBase> base = make_shared<CDerived>();
    }

    {
        cout << "dynamic lifetime 5" << endl;
        shared_ptr<void> base = make_shared<CDerived>();
    }

    {
        cout << "dynamic lifetime 6!!!" << endl;
        shared_ptr<CBase> base = shared_ptr<CBase>(static_cast<CBase*>(new CDerived));
    }
    return 0;
}

[alexey-malov]

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

[alexey-malov]

    CSolidBody(double density) { m_density = (density > 0) ? density : 0; };

Лучше в одну строчку реализацию не писать - быстрее работать не станет. Реализацию базового класса можно было бы переместить в .cpp файл Для инициализации полей в конструкторе следует использовать списки инициализации Здесь у тебя функция std::max подходит Аналогично в других классах

[alexey-malov]

string CParallelepiped::ToString() const
{
    string bodyType = "Parallelepiped:\n";
    string width;
    (width   += "\tWidth   = ") += to_string(m_width) += "\n";
    string height;
    (height  += "\tHeight  = ") += to_string(m_height) += "\n";
    string depth;
    (depth   += "\tDepth   = ") += to_string(m_depth) += "\n";
    string mass;
    (mass    += "\tMass    = ") += to_string(GetMass()) += "\n";
    string volume;
    (volume  += "\tVolume  = ") += to_string(GetVolume()) += "\n";
    string density;
    (density += "\tDensity = ") += to_string(GetDensity());

    return bodyType + width + height + depth + mass + volume + density;
}

Советую посмотре в сторону std::ostringstream, либо boost::format и использовать raw string literals:

string CParallelepiped::ToString() const
{
    /*
    return (boost::format(
R"(Parallelepiped:
    Width   = %1%
    Height  = %2%
    Depth   = %3%
    Mass    = %4%
    Volume  = %5%
    Density = %6%)") 
        % m_width 
        % m_height 
        % m_depth 
        % GetMass() 
        % GetVolume() 
        % GetDensity()
        ).str();
    */
    string bodyType = "Parallelepiped:\n";
    string width;
    (width   += "\tWidth   = ") += to_string(m_width) += "\n";
    string height;
    (height  += "\tHeight  = ") += to_string(m_height) += "\n";
    string depth;
    (depth   += "\tDepth   = ") += to_string(m_depth) += "\n";
    string mass;
    (mass    += "\tMass    = ") += to_string(GetMass()) += "\n";
    string volume;
    (volume  += "\tVolume  = ") += to_string(GetVolume()) += "\n";
    string density;
    (density += "\tDensity = ") += to_string(GetDensity());

    return bodyType + width + height + depth + mass + volume + density;
}

[alexey-malov]

Библиотечные классы лучше инклудить в stdafx.h, в этом случае компиляция будет выполняться быстрее

[alexey-malov]

stdafx.h следует инклудить только в .cpp-файлах. В .h их использовать не нужно

[alexey-malov]

class CCompound :
    public CBody
{
public:
    bool AddBody(std::shared_ptr<CBody> pNewBody);

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

[alexey-malov]

Вместо:

double CCompound::GetMass() const
{
    return accumulate(m_bodies.begin(), m_bodies.end(), 0.0,
    [](double sum, shared_ptr<CBody> const &body)
    { 
        return sum + body->GetMass();
    });
}

можно использовать диапазоны из boost

using boost::accumulate;
...
double CCompound::GetMass() const
{
    return accumulate(m_bodies, 0.0, 
    [](double sum, shared_ptr<CBody> const &body)
    {
        return sum + body->GetMass();
    });

Есть еще адапторы, применяемые к диапазону, такие как filter, transform, map_keys, map_values и др.

    typedef std::map<RequestId, BackgroundThumbnailRequest> BackgroundThumbnailRequests;
    BackgroundThumbnailRequests m_pendingThumbnailRequests;

    auto pendingRequestIds = m_pendingThumbnailRequests | map_keys;
    CancelPendingRequests({pendingRequestIds.begin(), pendingRequestIds.end()});

    void CancelPendingRequests(const std::vector<RequestId> & requestIDs){}

    auto clonedStartScene = scenes.GetStartScene() ? sceneClones.at(scenes.GetStartScene()) : nullptr;

    auto clonedScenes = sceneClones | boost::adaptors::map_values;
    return CScenes(CSceneStorage(clonedScenes.begin(), clonedScenes.end()), clonedStartScene);

        static void FillMessageIndexMapWithNewMessages(MessageToIndexMap& messageMap, Messages const& messages)
        {
            auto newMessages = messages | boost::adaptors::filtered([&messageMap](CMessage const& msg){
                return messageMap.find(msg) == messageMap.end();
            });

            boost::transform(newMessages, std::inserter(messageMap, messageMap.end()), [&messageMap](CMessage const& msg){
                return std::make_pair(msg, messageMap.size());
            });
        }

Собрать рекурсивно все файлы (но не папки) в некотором базовом каталоге и поместить из в мапу вида: "относительный путь" => "параметры файла":

void DoSomething()
{
    auto storageContent = make_iterator_range(recursive_directory_iterator(m_root), recursive_directory_iterator());
    auto IsFile = [](directory_entry const& entry) {
        return is_regular_file(entry.status());
    };
    auto filesOnly = storageContent | filtered(IsFile);

    transform(filesOnly, inserter(m_files, m_files.end()),
        [&rootPath](directory_entry const& entry){
        auto filePath = make_relative(rootPath, entry.path()).generic_wstring();
        return make_pair(filePath, CFileInfo());
    });
}

// Return path when appended to a_From will resolve to same as a_To
boost::filesystem::path make_relative(boost::filesystem::path a_From, boost::filesystem::path a_To)
{
    a_From = boost::filesystem::absolute(a_From); a_To = boost::filesystem::absolute(a_To);
    boost::filesystem::path ret;
    boost::filesystem::path::const_iterator itrFrom(a_From.begin()), itrTo(a_To.begin());
    // Find common base
    for (boost::filesystem::path::const_iterator toEnd(a_To.end()), fromEnd(a_From.end()); itrFrom != fromEnd && itrTo != toEnd && *itrFrom == *itrTo; ++itrFrom, ++itrTo);
    // Navigate backwards in directory to reach previously found base
    for (boost::filesystem::path::const_iterator fromEnd(a_From.end()); itrFrom != fromEnd; ++itrFrom) //-V683
    {
        if ((*itrFrom) != ".")
            ret /= "..";
    }
    // Now navigate down the directory branch
    ret.append(itrTo, a_To.end());
    return ret;
}

[alexey-malov]

boost::optional<BodyPointer> ReadBody(istream &input, ostream &output)
{   
    while (true)
    {
        output << "choose next body" << endl;
        output << USER_INPUT_BEGINING;

        string command = GetCommand(input);
        if ("sphere" == command)
        {
            output << "Adding a sphere" << endl;
            return ReadSphere(input, output);
        }
        else if ("para" == command)
        {
            output << "Adding a parallelepiped" << endl;
            return ReadParallelepiped(input, output);
        }
        else if ("cone" == command)
        {
            output << "Adding a cone" << endl;
            return ReadCone(input, output);
        }
        else if ("cylinder" == command)
        {
            output << "Adding a cylinder" << endl;
            return ReadCylinder(input, output);
        }
        else if ("compound" == command)
        {
            output << "Adding a compound body" << endl;
            return ReadCompound(input, output);
        }
        else if ("end" == command)
        {
            output << "Ending read bodies" << endl;
            return boost::none;
        }
        else
        {
            output << "Unknown command" << endl;
        }
    }
}

Можно сделать

map<string, function<BodyPointer(istream&, ostream&)>>

Тогда код будет более коротким. Можно будет сконфигурировать этот map в одном месте, а использовать в разных местах

[alexey-malov]

почему бы вместо optional body pointer не возвращать просто body pointer нулевой?

[alexey-malov]

    BOOST_FIXTURE_TEST_SUITE(can_add, createdCompound)
        BOOST_AUTO_TEST_CASE(solid_body)
        {
            BOOST_CHECK(compound.AddBody(make_shared<CCone>(0, 0, 0)));
        }

        BOOST_AUTO_TEST_CASE(compound_body)
        {
            BOOST_CHECK(compound.AddBody(make_shared<CCompound>()));
        }
    BOOST_AUTO_TEST_SUITE_END()

Недостаточно проверить return value, надо убедиться, что объект появился внутри действительно

[alexey-malov]

добавить тест, проверяющий, что в составной объект нельзя добавлять объекты, которые добавлены в объекты из другой иерархии

[alexey-malov]

баллы: 100*0,8=80