我最近开始重构一个我已经好几个月没接触过的项目。

最初的目的是要为我玩过的游戏Ogame做一个库存管理系统。在本游戏中，您将在一个充满星系的宇宙中玩耍，这些星系充满了系统，这些系统充满了行星。这些行星的坐标由三部分组成（星系，系统，系统中的位置）。通常的大小是9个星系，499个系统和每个系统15个行星，最多可容纳67365个行星。在游戏中），可以根据坐标或属性对行星进行排序。相关的属性是3个采矿设施和太阳能（发电厂）。

注意负值不应该存在。尽管我认为不输入这些内容是用户的责任，但我也没有进行检查。这是设计使然，但是可能是一个不好的做法。

简短的UI也是设计使然的。我认为它足够直观（尤其是因为我是当前的唯一用户），但是如果有可能以更优雅的方式实现相同的结果，我将全力以赴。

CSV（Galaxy.txt） ：

1:2:12;20;18;15;0;7;6;4;
1:5:12;20;18;11;0;6;5;3;
1:9:4;10;7;6;0;1;0;0;
1:9:8;17;14;8;0;3;2;1;
1:9:9;18;16;7;0;4;3;2;
1:10:5;9;6;2;0;2;1;0;
1:10:12;2;2;2;0;1;0;0;
1:11:11;19;16;10;0;5;3;1;
1:11:12;23;20;16;0;7;6;5;
1:12:4;2;2;2;0;0;1;0;
1:14:5;4;2;1;0;1;0;0;
1:15:5;20;18;10;0;5;3;1;
1:16:5;18;16;9;0;5;4;4;
1:20:7;24;21;6;0;8;7;2;
1:20:8;22;20;14;0;9;7;5;
1:20:9;23;22;13;0;8;7;4;
1:21:6;11;12;11;0;6;4;3;
1:21:7;23;20;13;0;7;6;3;
1:22:7;14;13;9;0;3;2;2;
1:25:6;19;18;12;0;7;6;3;
1:25:11;16;15;4;0;6;5;3;
1:27:12;12;11;8;0;6;4;2;
1:32:6;19;17;13;0;6;5;3;
1:38:7;22;21;12;0;8;7;6;
1:38:10;18;14;13;0;7;5;4;
1:39:5;18;15;10;0;6;5;4;
1:39:12;20;18;12;0;7;6;4;
1:40:6;9;8;7;0;3;2;2;
1:42:7;19;18;15;6;6;5;4;
1:43:11;19;19;14;5;7;7;7;
1:43:12;19;19;14;5;7;7;7;
1:45:4;20;18;16;0;8;7;7;
1:45:5;20;17;16;0;7;6;5;
1:45:9;15;15;12;0;5;5;4;
1:45:13;15;15;15;0;5;5;4;
1:46:12;11;11;3;0;2;2;1;
1:56:8;22;18;12;0;7;6;5;
1:59:6;23;21;21;0;8;7;6;
1:61:7;17;16;17;5;2;1;2;
1:62:4;14;13;13;0;4;4;2;
1:62:10;3;2;0;0;0;0;0;
1:63:5;10;8;4;0;4;3;2;
1:64:3;5;2;1;0;2;2;0;
1:64:4;21;15;12;0;8;6;3;
1:64:8;9;9;6;0;3;2;2;
1:65:5;14;9;4;0;5;3;1;
1:65:8;23;20;17;7;8;7;7;
1:65:9;23;21;16;7;8;7;7;
1:65:10;13;16;14;0;5;5;6;
1:65:11;24;22;16;7;9;7;7;
1:67:4;17;16;10;0;5;5;3;
1:67:5;17;16;9;0;5;5;2;
1:152:7;16;14;12;0;5;5;4;
1:153:6;23;22;17;0;8;7;9;
1:154:8;23;20;18;0;6;6;3;
1:292:8;24;19;11;0;7;6;3;
2:157:6;24;20;20;6;9;6;6;
2:157:7;23;20;20;6;8;6;6;
2:157:12;10;7;2;0;1;1;0;

我想我在大多数事情上都遵循了“单一责任原则”，尽管仍然有几个大型功能。
命名不是太糟糕，但可能会更好。
排序功能重复很多，这很糟糕。
此代码的可维护性因素很差。
我尝试在:的开头保留可用类和函数的概述。感觉过时了，因为可维护性与利润似乎并不值得。
我可能复制了太多数据。我如何传递变量可能会获得优化。我怀疑我没有使用足够的指针。

Main.cpp 

#ifndef MAIN_CPP
#define MAIN_CPP

/*

Project :   Universe
By      :   Mast
Date    :   August '14

structUniverse
--------
sName                                           :   std::string
vPlanet                                         :   std::vector<Planet> vPlanet;

vsSanitizeData(std::vector<std::string>)        :   std::vector<std::string>
sort_x();                                       :   void
sort_y();                                       :   void
sort_z();                                       :   void
sort_Mlvl();                                    :   void
sort_Clvl();                                    :   void
sort_Dlvl();                                    :   void
sort_Plvl();                                    :   void
readCSV();                                      :   void
addPlanet(Planet);                              :   void
splitString(std::string);                       :   int

getUniverse();                                  :   void
printUniverse();                                :   void


structPlanet
--------
x                                               : int
y                                               : int
z                                               : int
Mlvl                                            : int
Clvl;                                           : int
Dlvl;                                           : int
Plvl;                                           : int
Mstor;                                          : int
Cstor;                                          : int
Dstor;                                          : int

getPlanet();                                    : int
printPlanet();                                  : void
getMlvl();                                      : int
getClvl();                                      : int
getDlvl();                                      : int

*/

#include "Universe.hpp"

struct Planet;
struct Universe;

int main()
{
    Universe Uno("Uno");
    Uno.readCSV();

    bool exit = false;
    while (!exit)
    {
        system("cls");
        std::cout << "Welcome to the Galaxy Management Unit (GMU)." << '\n'
            << '\n'
            << "0. Print." << '\n'
            << '\n'
            << "1. Sort X." << '\n'
            << "2. Sort Y." << '\n'
            << "3. Sort Z." << '\n'
            << '\n'
            << "4. Sort Mlvl." << '\n'
            << "5. Sort Clvl." << '\n'
            << "6. Sort Dlvl." << '\n'
            << "7. Sort Plvl." << '\n'
            << '\n'
            << "999. Exit." << '\n'
            << '\n'
            << "Choice: "
            ;
        int choice;
        std::cin >> choice;
        switch (choice)
        {
        case 0:
            Uno.printUniverse();
            std::cin.sync();
            std::cin.clear();
            std::cin.get();
            break;
        case 1:
            Uno.sort_x();
            break;
        case 2:
            Uno.sort_y();
            break;
        case 3:
            Uno.sort_z();
            break;
        case 4:
            Uno.sort_Mlvl();
            break;
        case 5:
            Uno.sort_Clvl();
            break;
        case 6:
            Uno.sort_Dlvl();
            break;
        case 7:
            Uno.sort_Plvl();
            break;
        case 999:
            exit = true;
            break;
        default:
            break;
        }

    }

}

#endif


Universe.hpp 

#ifndef UNIVERSE_HPP
#define UNIVERSE_HPP

/*

Project : Universe
By      : Mast
Date    : August '14

*/

#include <algorithm>
#include <iostream>
#include <string>
#include <fstream>
#include <vector>

struct Planet
{
    int x;
    int y;
    int z;
    int Mlvl;
    int Clvl;
    int Dlvl;
    int Plvl;
    int Mstor;
    int Cstor;
    int Dstor;

    Planet(int x, int y, int z, int Mlvl, int Clvl, int Dlvl, int Plvl, int Mstor, int Cstor, int Dstor) : x(x), y(y), z(z), Mlvl(Mlvl), Clvl(Clvl), Dlvl(Dlvl), Plvl(Plvl), Mstor(Mstor), Cstor(Cstor), Dstor(Dstor) {}

    ~Planet(){};

    int getPlanet();
    void printPlanet();
    int getMlvl();
    int getClvl();
    int getDlvl();
};

int Planet::getPlanet()
{
    return x, y, z, Mlvl, Clvl, Dlvl, Plvl, Mstor, Cstor, Dstor;
}

void Planet::printPlanet()
{
    std::cout << x << ':'
        << y << ':'
        << z << ' '
        << Mlvl << ' '
        << Clvl << ' '
        << Dlvl << ' '
        << Plvl << ' '
        << Mstor << ' '
        << Cstor << ' '
        << Dstor
        << '\n';
}

int Planet::getMlvl()
{
    return Mlvl;
}

int Planet::getClvl()
{
    return Clvl;
}

int Planet::getDlvl()
{
    return Dlvl;
}

struct Universe
{
    std::string sName;
    std::vector<Planet> vPlanet;

    Universe(std::string sName) : sName(sName) {}

    ~Universe(){};

    std::vector<std::string> vsSanitizeData(std::vector<std::string>);
    void sort_x();
    void sort_y();
    void sort_z();
    void sort_Mlvl();
    void sort_Clvl();
    void sort_Dlvl();
    void sort_Plvl();
    void readCSV();
    void addPlanet(Planet);
    int splitString(std::string);

    void getUniverse();
    void printUniverse();
};

struct compare_x
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.x < right.x);
    }
};

struct compare_y
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.y < right.y);
    }
};

struct compare_z
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.z < right.z);
    }
};

struct compare_Mlvl
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.Mlvl < right.Mlvl);
    }
};

struct compare_Clvl
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.Clvl < right.Clvl);
    }
};

struct compare_Dlvl
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.Dlvl < right.Dlvl);
    }
};

struct compare_Plvl
{
    inline bool operator() (const Planet& left, const Planet& right)
    {
        return (left.Plvl < right.Plvl);
    }
};

void Universe::sort_x()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_x());
}

void Universe::sort_y()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_y());
}

void Universe::sort_z()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_z());
}

void Universe::sort_Mlvl()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_Mlvl());
}

void Universe::sort_Clvl()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_Clvl());
}

void Universe::sort_Dlvl()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_Dlvl());
}

void Universe::sort_Plvl()
{
    std::sort(vPlanet.begin(), vPlanet.end(), compare_Plvl());
}

std::vector<std::string> Universe::vsSanitizeData(std::vector<std::string> Vec)
{
    std::vector<std::string>::iterator vI = Vec.begin();
    while (vI != Vec.end())
    {
        if (*vI == "")
        {
            vI = Vec.erase(vI);
        }
        else
        {
            vI++;
        }
    }

    return Vec;
};

void Universe::readCSV()
{
    std::ifstream input("Galaxy.txt", std::ios::in);
    std::string sRead;
    std::vector<std::string> sData;
    std::vector<std::string>::iterator itData;
    while (input.good())
    {
        getline(input, sRead);
        sData.push_back(sRead);
    }
    sData = vsSanitizeData(sData);
    for (itData = sData.begin(); itData != sData.end(); itData++)
    {
        splitString(*itData);
        std::cout << '\n';
    }

}

void Universe::addPlanet(Planet p)
{
    vPlanet.push_back(p);
}

void Universe::getUniverse()
{
    for (unsigned i = 0; i < vPlanet.size(); i++)
    {
        vPlanet[i].getPlanet();
    }
}

void Universe::printUniverse()
{
    std::cout << '\n';
    for (unsigned i = 0; i < vPlanet.size(); i++)
    {
        vPlanet[i].printPlanet();
    }
}

int Universe::splitString(std::string sSplitInp)
{
    char primaryLimiter = ';';
    char secondLimiter = ':';
    std::string sTemp;
    std::vector<int> vTemp;

    for (size_t p = 0, q = 0; p != sSplitInp.npos; p = q)
    {
        sTemp = sSplitInp.substr(p + (p != 0), (q = sSplitInp.find(primaryLimiter, p + 1)) - p - (p != 0));
        std::cout << sTemp << ' ';
        if (p == 0)
        {
            std::string sNew;
            size_t j = 0, k = 0;
            for (unsigned i = 0; i <= 2; i++)
            {
                sNew = sTemp.substr(j + (j != 0), (k = sTemp.find(secondLimiter, j + 1)) - j - (j != 0));
                vTemp.push_back(atoi(sNew.c_str()));
                j = k;
            }
        }
        else
        {
            vTemp.push_back(atoi(sTemp.c_str()));
        }
    }
    addPlanet(Planet(vTemp[0], vTemp[1], vTemp[2], vTemp[3], vTemp[4], vTemp[5], vTemp[6], vTemp[7], vTemp[8], vTemp[9])); // ugly

    return 0;
}

#endif


#1 楼



在Main.cpp中不需要包含保护罩


#ifndef MAIN_CPP
#define MAIN_CPP




<头文件不会被编译器/预处理器多次包含（#include的工作方式与文本复制粘贴一样，因此，预处理器就是愚蠢的）。拥有get*方法的所有成员都可以公开访问，这很愚蠢。但是，应注意，“ getter”通常是const方法，以允许在const实例上调用它们，并在编译时强制不能在方法内部修改成员类数据。例如：

void printPlanet() const;
int getMlvl() const;


还请注意，printPlanet()不是吸气剂，但它也属于仅从成员数据读取的类别。 >我建议将类实现从Universe.hpp移到Universe.cpp文件中。
Main.cpp中的摘要注释是多余的，并使用Universe.hpp中的声明重复了自己。如上所述，如果将实现部分移动到CPP文件，则阅读声明会容易得多，因此不需要任何额外的摘要或文档。
如果您对C ++ 11开放，那么您可以用内联lambda替换所有这些小的比较器结构。这将使排序谓词与sort调用保持在一起。对读者来说更好的“参考位置”。
应省略空的析构函数，并让编译器提供默认值。


评论

\ $ \ begingroup \ $
成员不应该公开的。我一定会研究C ++ 11 lambda的，听起来像是一个很好的借口，终于可以学习这些了。
\ $ \ endgroup \ $
–桅杆
15年6月28日在18:06

#2 楼

我对您的设计有一些疑问。

类

如果所有数据和方法都是公共的，使用类的目的是什么？为什么要麻烦使用诸如getMlvl()，getClvl()，getDlvl()这样的吸气剂和吸气剂？就个人而言，我会将所有实例变量都设为私有，并保留访问器，因为如果以后进行扩展，否则将很难跟踪谁在更改对象的一部分。但是，如果您要将它们全部公开，则不必费心使用访问器。 （或者至少要保持一致，并让所有成员都不要这样做。）

Planet::getPlanet()应该做什么？它返回一个int，但是return语句有10个似乎正在返回的变量。我的编译器对此感到困惑。似乎您正在尝试复制它？就像这样：

Planet Planet::getPlanet()
{
    return Planet(x,y,z,Mlvl,Clvl,Dlvl,Plvl,Mstor,Cstor,Dstor);
}


但是，如果这就是您的意图，那么也许您真正需要的是复制构造函数？ Planet类似乎不必要。只需将其设置为仅包含struct的int和单个非类函数printPlanet(const Planet&)即可正常工作。您的Universe类包含Planet的单个向量。我以为宇宙有一个层次结构，包含星系，包含系统，包含行星？但是我只看到Universe包含Planets。这似乎很奇怪。

命名

我不太喜欢匈牙利符号。但是，如果要使用它，请保持一致。 Planet中的所有成员变量都没有使用它。我希望它们被命名为iX，iY，iZ等。尤其是因为坐标通常是浮点数。同样在Universe中，您还有vector<Planet> vPlanet。我希望它是vpPlanet，并且我可能会使它复数，因为它通常容纳1个以上（除非它是一个孤独的Universe）。

与vsSanitizeData()的参数相同。应该将其命名为vsData或更具描述性的名称，例如vsToSanitize。我也不了解Clvl，Dlvl或Mlvl是什么。不要害怕使用全名，例如Plvl或Mstor（或它们代表的任何名称）。具有将比较结构作为其参数的1或可能是一个枚举，说明要进行哪种排序。

Cstor具有返回值，但始终为0，无论如何都将被忽略。另外，它不只是拆分字符串。它构造一个新的Dstor并将其添加到miningLevel中。这似乎比分割字符串要大得多，所以我会更改名称以反映这一点。也许powerLevel？将字符串设为const引用将使您无法复制字符串。

#3 楼

关于CSV格式和阅读器，我建议更改以下格式：


1：2：12; 20; 18; 15; 0; 7; 6; 4;


到


“ 1：2：12”; 20; 18; 15; 0; 7; 6; 4;


如您所见，我将行星坐标视为字符串，而不是现在的处理方式，这将消除同时使用:和;分隔符的需求。

评论

\ $ \ begingroup \ $
由于我想对字符串的所有三个部分分别进行排序，这会不会增加复杂性？具有两个定界符是相对较小的事情，因为它们不会很快改变。
\ $ \ endgroup \ $
–桅杆
2015年6月28日19:19


\ $ \ begingroup \ $
@Mast您可以解析字符串中的整数，然后对它们进行排序，CSV只是一种数据格式，应保持简单
\ $ \ endgroup \ $
–skiwi
15年6月28日在19:20
\ $ \ begingroup \ $
你是对的。我不应该只是为了避免使用其他方法而使数据格式变得不必要地复杂。
\ $ \ endgroup \ $
–桅杆
15年6月28日在19:22
\ $ \ begingroup \ $
@Mast在字符上分割字符串并不难。
\ $ \ endgroup \ $
–马克·安德烈（Marc-Andre）
15年6月29日在17:36
\ $ \ begingroup \ $
@ Marc-Andre不，不是。我已经在使用CSV阅读器进行此类操作。但是，它将拆分一个子字符串，因此复杂性增加了。
\ $ \ endgroup \ $
–桅杆
15年6月29日在17:39

#4 楼

这不是一个完整的评论，而是对已经提出的一点的详尽阐述。使用多种方式对事物进行排序并不是一个坏主意，但是有更好的方法来进行分类。我认为，这是三种可以做得更好的不同方法。

使用lambda

正如@glampert已经说过的那样，将排序例程更好地表示为一个lambda：

void Universe::sort_x() { 
    std::sort(vPlanet.begin(), vPlanet.end(), 
        [](const Planet &a, const Planet &b){ 
            return a.x < b.x; 
        }); 
}


但是，必须为Planet的每个成员变量都写出来有点麻烦，所以这引出了另一种方法：

使用宏

#define sort_on(x) sort_##x() { std::sort(vPlanet.begin(), vPlanet.end(), [](const Planet &a, const Planet &b){ return a.x < b.x; }); }

void Universe::sort_on(x)
void Universe::sort_on(y)
void Universe::sort_on(z)
void Universe::sort_on(Mlvl)
void Universe::sort_on(Dlvl)
void Universe::sort_on(Plvl)
void Universe::sort_on(Clvl)


现在已定义了所有成员函数，但仍具有一定可读性。但这仍然有些不幸，因为这意味着Universe必须具有对Planet的内部成员的深入了解和访问。通常这是一个危险信号，告诉您类设计可能有问题。

Planet类中的实现比较

您可以将比较实现作为静态成员使用Planet类别的

一个将这个Planet作为参数并返回适当的函数指针的函数。

static bool byX(const Planet &a, const Planet &b) { return a.x < b.x; };
// etc.


然后，在enum中将不再具有多个排序函数，而只有一个：

enum sortby { X, Y, Z, MLVL, CLVL, DLVL, PLVL };


还有一些想法

我建议您使用标准的流提取器和插入器，而不是执行所有复杂的I / O。这样做时，代码将大大缩水： />
Main.cpp 

static bool (*sorter(sortby field))(const Planet &a, const Planet &b) {
    switch (field) {
        case Y:
            return byY;
            break;
        case Z:
            return byZ;
            break;
        // all of the other cases
        default:
            return byX;
    } 
}


在对代码进行的许多更改中：


不要硬编码文件名
将I / O委托给正在读取的对象
使用C ++ 11 range-for简化打印集合
在实用的地方使用Universe
在实用的地方将字符串合并为单个


评论

\ $ \ begingroup \ $
您的最终解决方案似乎是对我的分类混乱的最终答案。可扩展，可维护，超赞！
\ $ \ endgroup \ $
–桅杆
15年6月29日在18:23

#5 楼

在vsSanitizeData()内，使用向量存储输入记录数据可能会变得非常低效。由于向量在内部使用数组，因此，当您移除向量中的一个元素时，移除一个元素之后的所有元素都必须移动。对于包含许多删除操作的长列表，这可能是很多不必要的工作。

作为设计的问题，我会在读取循环内尽早清理记录。在列表中插入不可用的记录没有意义。您可能还有其他拒绝记录的标准，这将是解决这些问题的好地方。

就风格而言，我同意关于匈牙利表示法及其烦恼的较早答案。我相信用类型指示符修饰变量名是没有意义的。当代码随时间粗心地重构时，它是多余的，并且会变得不准确，因此无论如何都不能依赖它。最好使用叙述性但又简单明了的名称来描述问题域。例如，我将使用Uno代替universe，而不是vPlanet。编码正在重新编码。