6
\$\begingroup\$

This is notepad that works with list of files (pages w/e) that can be edited (list). You can access each file separately and edit/view it too (rewrite, append, read). Here's my implementation of it. I have 2 classes that resemble menus (list of files, and file itself) both are inherited from interface and 1 class that edits files. Now the questions are:

  • How spaghetti is my code
  • Am I using interface in a right way here (is it needed)?
  • Is there better way to implement mode selection? (I don't like it at all)
  • What causes getline get empty line(s) before I send it to append() (I have to check for them)

Any other criticism is appreciated.

Menu.h

#pragma once
class Menu
{
private:
    virtual int input() = 0;
public:
    Menu() {}
    Menu(const char*) {};
    virtual int output() = 0;
    virtual ~Menu() {}
};

FileMenu.h

#pragma once
#include "Menu.h"
#include "File.h"
#include <memory>

class FileMenu :
    public Menu
{
private:
    std::unique_ptr<File>m_f;
    int input();
public:
    FileMenu();
    FileMenu(const char*);
    ~FileMenu();
    int output();
};

FileMenu.cpp

#include "FileMenu.h"
#include <iostream>
#include <string>


FileMenu::FileMenu(){}
FileMenu::FileMenu(const char* path) :m_f(std::make_unique<File>(path)) {}
FileMenu::~FileMenu()
{
    //delete m_f;
}

int FileMenu::input()
{
    std::string mode;
    std::cin >> mode;
    if (!mode.compare("a"))
    {
        std::string buff;
        while (buff.compare("/0")) //end of edit
        {
            if (buff.compare("")) //idk how to fix (if i use getline before append it will still get empty line and will write /0 to file (have to do another if)
            {
                m_f->append(buff.c_str());
                //m_f->append("\n");
            }
            std::getline(std::cin, buff);

        }
        return 0;
    }
    if (!mode.compare("w"))
    {
        std::string buff;
        m_f->clear();
        while (buff.compare("/0"))
        {
            if (buff.compare(""))
            {
                m_f->append(buff.c_str());
                //m_f->append("\n");
            }
            std::getline(std::cin, buff);
        }
        return 0;
    }
    if (!mode.compare("r"))
    {
        m_f->output(m_f->read());
        system("pause"); //bad
        return 0;
    }
    if (!mode.compare("e"))
    {
        return 1;
    }

    return -1;
}

int FileMenu::output()
{
    do
    {
        system("cls");
        std::cout << "\t(w)rite (a)ppend (r)ead (e)xit" << std::endl;
    } while (input() != 1);
    return 0;
}

MainMenu.h

#pragma once
#include "Menu.h"
#include "File.h"
#include <memory>

class MainMenu :
    public Menu
{
private:
    std::unique_ptr<File>m_f;
    int input();
public:
    MainMenu();
    MainMenu(const char*);
    ~MainMenu();
    int output();
};

MainMenu.cpp

#include "MainMenu.h"
#include "FileMenu.h"
#include "File.h"

#include <iostream>
#include <string>
#include <algorithm>
#include <cctype>

MainMenu::MainMenu(){}
MainMenu::MainMenu(const char * path) :m_f(std::make_unique<File>(path)) {}
MainMenu::~MainMenu(){}

int MainMenu::input()
{
    std::string mode;
    std::cin >> mode;
    if (std::find_if(mode.begin(),
        mode.end(), [](char c) { return !std::isdigit(c); }) == mode.end()) //check if mode is number 
        {
            std::string pathBuff;
            std::ifstream f = m_f->read();
            f.clear();
            f.seekg(0, std::ios::beg);
            for (int i = 1; i < std::strtol(mode.c_str(), nullptr, 0);i++) //iterate through list of files
            {
                std::string token;
                getline(f, token);
            }
            getline(f, pathBuff);
            std::unique_ptr<Menu> fm(std::make_unique<FileMenu>(pathBuff.c_str())); //go to file menu
            fm->output();
        }
    if (!mode.compare("n"))
    {
        std::string path;
        std::cin >> path;
        m_f->append(path.c_str()); //add new file to list
        std::unique_ptr<File> f (std::make_unique<File>(path.c_str()));
        f->create(); //create new file
        std::unique_ptr<Menu> fm (std::make_unique<FileMenu>(path.c_str())); //go to file menu
        fm->output();
        return 0;
    }
    if (!mode.compare("e"))
    {
        return 1;
    }

    return -1;
}

int MainMenu::output()
{
    do
    {
        system("cls");
        m_f->output(m_f->read());
        std::cout << "\t(1..) open file (n)ew file (e)xit" << std::endl;

    } while (input() != 1);
    return 0;
}

File.h

#pragma once
#include <fstream>

class File
{
private:
    const char* m_path;
public:
    //File();
    File(const char*);
    int create();
    int append(const char*);
    int clear();
    std::ifstream read();
    void output(std::ifstream);
    ~File();
};

File.cpp

#include "stdafx.h"
#include "File.h"
#include <iostream>
#include <string>

File::File(const char* path):m_path(path){}

File::~File()
{
    delete m_path;
}

int File::create()
{
    std::ofstream out(m_path);
    return 0;
}

int File::append(const char* str)
{
    std::ofstream out(m_path, std::ios::app);
    out << str <<'\n';
    return 0;
}

int File::clear()
{
    std::ofstream out(m_path, std::ios::trunc);
    return 0;
}

std::ifstream File::read()
{
    std::ifstream in (m_path);
    return in;
}

void File::output(std::ifstream in)
{
    std::string lineBuff;
    while (getline(in, lineBuff))
    {
        //if (lineBuff.compare(""))//idk how to fix empty lines
        {
            std::cout << lineBuff << std::endl;
        }
    }
}

main()

int main()
{
    {   std::unique_ptr<Menu> menu (new MainMenu("inf.f"));
        menu->output();
    }
    /*if (_CrtDumpMemoryLeaks())
        std::cout << "memory leak!" << std::endl;*/
    return 0;
}
\$\endgroup\$
3
\$\begingroup\$

Overall

  • Most of your constructors and destructors are defaulted. just use = default;

    class Menu { Menu() = default; /*...*/ };
    
  • You are taking char* in a few places. Prefer std::string. A few places you convert the char* to a unique_ptr, just make the unique_ptr part of your interface. A raw pointer means, "Here is a reference to something. I am not giving you ownership". A unique_ptr<> means, "Here is something. You are responsible for it now. You own it." FileMenu is a perfect example. The path is owned by FileMenu. It creates the file and does stuff with it. So it should take a unique_ptr<>.

  • A few places you are checking for "/0". Is that what you meant for "mode-change"? If it is, that's very strange. I would steal mode change from a popular editor like escape for vim or some ridiculous ctrl chord for emacs. Then your users will be able to guess the correct semantics. -- If you meant it as NULL then it isn't correct. "/0" This is a const char[2], i.e., a literal backslash and a zero. You probably meant the representation for null '\0' which is actually a single character. However, it's best to avoid this anyway.

  • There are a lot of while loops. I would prefer to see more algorithms.

  • Try to localize all I/O to one point in the program, then feed that input to a parser who directs commands to the FileMenu or MainMenu, or File respectively. This also allows you to unit test the command interfaces without mocking I/O.

FileMenu::input()

  • You are reading from standard-input in multiple places. I can image this makes it hard to debug. I find it easier to do all of my I/O in one place, then process that I/O.
  • I think this function is missing 2 classes. An input reader, and an input parser. The input reader reads characters and feeds them to the parser. The Parser recognizes if the string is a command or random input. Do this will separate the I/O from the logic. (Flex and Bison work this way, but you do not need something so complex.)

However just a simple refactor for this class is to convert the return type from int to an enum. Then FileMenu::output changes to looking for a CLOSE enum rather than a "floating-invariant" of !1 means exit.

FileMenu::Output

If you had the input class, and parser class as mentioned above, this function could install commands (perhaps enums) into the parser class to tell it what commands to expect.

File::File

This class takes a raw pointer and deletes it. This should be a unique pointer since the delete implies that File owns the object. However, this means that someone else is calling new on that pointer. If the pointer isn't a heap object, and you try to delete it, bad things happen. The pointer is ambiguous about its lifetime.

\$\endgroup\$
  • \$\begingroup\$ By mode change I meant read write append, etc. I used "/0" to exit from edit mode. \$\endgroup\$ – mizuki_ Sep 30 '15 at 22:04
2
\$\begingroup\$

I only have a few points to add to @JeremyWright's review:

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif
\$\endgroup\$
  • \$\begingroup\$ Isn't it generally the user's responsibility to prevent someone unauthorized from placing commands in their PATH? If the user deliberately creates those commands then it's not really a security hole by the airtight hatchway principle. \$\endgroup\$ – Random832 Oct 1 '15 at 4:28
  • \$\begingroup\$ Rather than setting a trap for users to fall into, the better security stance is to avoid the pitfall entirely. \$\endgroup\$ – Edward Oct 1 '15 at 12:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.