HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcppMinor

Some sort of console "notepad"

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sortconsolenotepadsome

Problem

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 

class FileMenu :
    public Menu
{
private:
    std::unique_ptrm_f;
    int input();
public:
    FileMenu();
    FileMenu(const char*);
    ~FileMenu();
    int output();
};


FileMenu.cpp

```
#include "FileMenu.h"
#include
#include

FileMenu::FileMenu(){}
FileMenu::FileMenu(const char* path) :m_f(std::make_unique(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(""))

Solution

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.

Code Snippets

class Menu { Menu() = default; /*...*/ };

Context

StackExchange Code Review Q#106190, answer score: 3

Revisions (0)

No revisions yet.