Intermediate programmer server

[u/Acrobatic-Tutor7745]

I'm an intermediate 17 y.o programmer, and I've made this server code. Idk what to do with it yet, but would like to know how it is, I'm very proud of it, but would like an outsider view. Also, if you'd like to contribute go ahead, but I still haven't decided if I want to use it commercially in the future, so take that into consideration. Anyways, how is it? I want an absolutely honest opinion.

Link;
https://github.com/itamaroren8/GenericAppServer.git

[u/mredding]

class Communicator {
private:

Classes are private access by default, so this is redundant.

SqliteDatabase* _db;

You likely want to use a smart pointer. Don't get fancy with naming conventions - they're ad-hoc type systems. You already have a type system inherent to C++. _db isn't a member of Communicator because it has a little leading underscore, it's a member because it's a member. Your IDE will tell you it's a member, what, and where. Any other contributor or maintainer can diverge from your convention and the language itself will not protect you. It's a lot of work for no payoff.

std::vector<LoggedUser> _loggedUsers;

You didn't #include <vector>, don't make a habit of taking advantage of transient includes. Should the offending header remove its dependency, this code then also breaks.

#endif //APP_COMMUNICATOR_H

Again, your IDE can tell you what conditional macro is ending. Don't use comments to tell me what the code tells me, or what other tools will already - and more correctly, tell me.

C++ source code ends with an empty newline.

---

Communicator::Communicator() {
  _db = new SqliteDatabase(); // Interchangeable to any implemented database class
  _db->open();
}

Your comment is wrong. It's not interchangeable, it's hard coded. If you wanted interchangeable, you would have implemented a parameterized constructor:

explicit Communicator(std::unique_ptr<IDatabase>);

You have a memory leak, because you have here a new but no corresponding delete, which would belong in the destructor. If you use a smart pointer, you don't have to worry about it.

You should use your initializer list:

Communicator::Communicator(): _db{new SqliteDatabase{}} {
  _db->open();
}

---

A class implements and enforces an invariant. An invariant is a statement that is always true whenever a client observes the instance of a class. For example - std::vector is always implemented in terms of 3 pointers, and those pointers always have a particular, consistent relationship. YOU can never observe a vector in an indeterminate state, where the pointers are somehow wrong.

A class implementation can suspend the invariant - for example, when std::vector::push_back reallocates, those internal pointers are - for a moment, inconsistent; they point to TWO different allocations for a time while they get reassigned. But the invariant is reestablished before control is returned. push_back CANNOT return through any code path before the invariant is reestablished.

And so we get to RAII. Resource Acquisition Is Initialization. It's horribly named. What it means is constructors ARE NOT factories. They do not BUILD the object. They merely take ownership of resources, and THAT inherently initializes the object.

This means the invariant must be established by the end of the initializer list, BEFORE you enter the ctor body. You can suspend the invariant therein, but you must reestablish it again before completing.

RAII is an idiom. The language cannot force it upon you, you have to follow it by convention. It's a better convention to follow than a naming convention...

So what you should do is write that parameterized constructor, and then use a factory to create the database and instantiate the Communicator.

And the invariant nature of a class means A) anything that is variant doesn't belong in a class. A car can start, stop, turn... And the members maintain that state. A car does not depend on the make, model, year, color... You can associate those properties with another data structure, like a table. Classes are terrible buckets for random bits of data. B) No getters or setters. Members are an implementation detail that no one wants to know about. Cars don't get_speed, they accelerate. Further, if you can change the invariant state, you are subverting the invariant nature of what a class is - you have a data structure. You NEVER want your class instance to be able to get into an inconsistent or indeterminate state. An instance should never be able to be born in an indeterminate or invalid state. The whole point is the interface implements all the valid state transitions.

---

std::thread workerThread([this,  socket = std::move(sock.release())]() mutable {handleClient(std::move(socket));});
workerThread.detach();

One thread per socket doesn't scale. This is why the Apache web server had to abandon v1.x, because they tried this and failed. I mean, kudos to you for getting so deep as to play with socket programming, but no production server does it this way.

What you do is you use the socket API, like select or more modern epoll to check a list of sockets, to see which ones are ready. If they're ready, they have data. THOSE can get dispatched to a worker pipeline. UDP sockets are easy, because the data on it is going to be whole datagrams, you can just read and process off the socket until you drain the socket buffer of datagrams. You'll never see a split datagram. TCP is a little different, you can read and process what data is available, but the data available may be incomplete, so you have to be able to stash your intermediate result until the socket is ready again. You won't know until you start draining the socket buffer to see how much is on there, and you don't know if or when more data is going to ever come.

What you don't do is perform IO on a thread.

Instead of detaching threads, use child processes.

What about my shared resources?

Your what, now?

---

IRequestHandler* requestHandler = new LoginRequestHandler(_db, _loggedUsers);

Smart pointers. All the way. The language gives you primitives so you can build up expressiveness. We have raw pointers so we can build smart pointers. We have smart pointers so we can make our code exception safe and defer to our more basic types to do these pedantic tasks for us. These layers never leave the compiler. Languages are greater than the machine code their compilers generate.

while (true) {

Loops also have invariants. When the invariant is invalidated, we exit the loop. The loop invariant here is we NEVER EXIT THE LOOP.

break;

And yet we do. So tell me, how is a compiler supposed to reason about this loop and optimize it if it can't depend on the invariant?

In other words, you want to express your loops in terms of an invariant that can be invalidated. If the loop can terminate, then write it as such.

IRequestHandler requestHandler{_db, _loggedUsers}; // Not everything has to be dynamically allocated...
char buffer[BUFFER_SIZE];

for(std::ssize_t n; n = socket.read(buffer, sizeof(buffer)).value() > 0; do_work(requestHandler, buffer, n));
std::cout << "Client disconnected!\n";

Your else condition is everything that can happen outside the loop anyway, since it can only execute once and break out of the loop - it's a better place to put one-time code, not inside a loop.

There's a good way to write forever loops - they have a return inside the loop body. This isn't the same thing, semantically, as breaking. When you return - there is no loop. There is no loop invariant. It's all gone. So there's no contradiction that can be had.

---

class JsonDeserializer {
public:
  static IRequest* deserializeLoginRequest(const std::vector<char>& buffer);
};

Let me fix this for you:

IRequest* deserializeLoginRequest(const std::vector<char>& buffer);

Not everything has to be a class.

---

typedef struct {
  std::string username;
} LoggedUser;

Making your own types, even like this == good.

This typedef thing here is a C artifact. It's entirely moot in C++.

In C, struct is not an optional keyword. Every time you refer to a structure, it must be prepended with struct:

struct foo {};

struct foo instance;

Using a type alias binds the type semantics to the alias symbol.

struct foo {};
typedef struct foo foo;

foo instance; // `foo` alias implies `struct foo`.

This is a foo alias to a struct foo. We can condense:

typedef struct foo {} foo;

foo instance; // `foo` alias implies `struct foo`.

You don't HAVE TO name a structure, especially if the structure doesn't refer to itself, so:

typedef struct {} foo;

foo instance;

Mind you - this ISN'T a foo structure anymore, it's a foo alias to an unnamed structure.

The C parser is dead simple, because it originally had to run on a PDP-7, so it sacrificed your convenience just to fit on the platform. By the time C++ came around, hardware came along, and being a new language, didn't have to pay this tax.

So what you have in your code is redundant. Just write:

struct LoggedUser { std::string username; };

---

deserializeLoginRequest(const std::vector<char> &buffer)

JSON is a string of text, not an array of characters. Your semantics are wrong. You should get the conversion from raw bytes to text earlier. Your interface is a contract - I take text and marshal it into a JSON object, or throw an exception trying!

This whole function can be reduced.

First, there's a philosophy that indentation is a good excuse to write a function:

switch(data["code"]) {
case Login: do_login(); break;
case SignUp: do_signup(); break;
default: throw;
}

Second, nlohmann/json explains how to support serialization of your own types. Your code is doing exactly what the library tells you not to do. You probably want to follow their documentation for type support. "code" isn't just an int, it's actually a LoginRequestCodes, which you want to explicitly extract with the library's get method, and fail earlier, while marshaling. You also want your request types to extract themselves.

---

There's lots I would change in this code, especially to align with types and objects, but I think these are the lessons for now.

[u/Acrobatic-Tutor7745 → u/mredding]

Wow, I really appreciate you for taking the time. I didn't know most of what you've written, so I probably have a lot to learn. I'll try to fix my code accordingly. Thanks a lot for reviewing it so thoroughly :)

[u/mredding → u/Acrobatic-Tutor7745]

r/cpp_questions

[u/Acrobatic-Tutor7745 → u/mredding]

Thanks! I'll try to improve my code in the next few days.

[u/gmes78 → u/mredding]

#endif //APP_COMMUNICATOR_H

Again, your IDE can tell you what conditional macro is ending. Don't use comments to tell me what the code tells me, or what other tools will already - and more correctly, tell me.

My IDE (CLion) generates those comments when creating header files. This is a fairly standard thing to do.

[u/mredding → u/gmes78]

My IDE (CLion) generates those comments when creating header files. This is a fairly standard thing to do.

I already know that, on both counts, and I don't care, on both counts. While I appreciate being lazy and not bothering to configure the IDE, the comment assumed OP was hand writing the code.

[u/gmes78 → u/mredding]

I'm just pointing out that the thing you're marking as bad is standard across many real world projects.

[u/mredding → u/gmes78]

I know, and you're supporting the Ad Populum fallacy; just because a bunch of projects do it, that doesn't mean it's either A) good, or B) and to my point - necessary.

Sure, back when we were using dumb editors, annotating your code this way was helpful because you had nothing else. I can tell you from my experience that this behavior is a holdover from the era before IDEs. Now your most basic tools can annotate your code for you with a pop-up tool tip.

[u/Acrobatic-Tutor7745 → u/mredding]

Clion added it automatically, and I see no point of deleting it. Even if it's not worth adding, does it truly hurt when it's there, that you actively need to remove it?

[u/gmes78 → u/mredding]

It would only be a negative if it required effort to write. Seeing as it's entirely automated, doesn't clutter the source code, and is useful when not inside an IDE, I don't see why one should remove it.

[u/mredding → u/gmes78]

It would only be a negative if it required effort to write.

False. It's a negative now that I have to maintain it. The comment was only generated from a template, it's structure is not automated, it's maintenance is not automated. You've just saddled me with shit.

Rename the file. Change the guard. Now I have to update the comment, too? Now do that 100x.

I don't need comments to tell me what the code tells me. Imagine:

void fn() {
  x *= 2; // Cube x
}

Where is the error? Is it in the code or the comment? Who is the authority? Which is to be changed? Why does this error exist in the first place?

What you're suggesting is more of the same. The auto-generated comment tells me what the code tells me, it does what active automated tools do better, it's redundant, and it's been outmoded for almost 20 years.

and is useful when not inside an IDE

IN WHAT WORLD are you looking at code not in an IDE? Are you printing code on a tractor fed dot-matrix printer? Even web tools like GitHub give you source metadata.

Are you telling me that your code is so absolutely massive that what, you get lost? You don't inherently know what the #endif refers to? That you can't trivially glance and tell? That sounds like your code is bloated and poorly maintained, that you've got bigger problems. You're using the comment as a landmark to know where you are in it and where things belong. Yikes.

So you're asking me to either A) maintain more code that doesn't service me to begin with, or B) disable this in the file template and solve the problem once and forever.

Man, you're REALLY trying to cling to the past and justify why you're still stuck in it. I've been writing code for 37 years, and as soon as IDEs started getting robust enough to give you code metrics and meta-data, I was right there to adopt it and abandon all this shit. Less is more. Move on.

[u/Alternative_Corgi_62 → u/mredding]

While your comments are mostly in place, I will fight you on "Your IDE will tell that...". I want to see what this #endif is closing without putting mouse cursor over the line. I want ten years from now to recall why I wrote "c *= 2". And so on...

[u/mredding → u/Alternative_Corgi_62]

You can move the cursor with the keyboard, too. If your code is so big you can't instantly know what the endif belongs to, you've got bigger problems, and that comment isn't the solution.

WHY is EXACTLY what comments are for. Why did you write c *= 2? Only a comment can tell. But WHAT and HOW are the domain of source code, and elegant source code reads well, is expressive, and concise.

[u/Extension_Unit3807 → u/mredding]

I smell something from someone who doesn't know what he's yapping

[u/johnpeters42]

First off, I would add a README file explaining what it's a generic app server for. (Apparently some sort of generic framework to log in and access a SQLite database. Framework for what type of app? Desktop app, mobile app, web app, API layer?)

Second, if you're thinking of going commercial, then what differentiates this from existing software in the same space, besides "this one is mine"? (Pro: One less dependency on third-party software. Con: You need to maintain this thing yourself.) Also, do you want to spend extra time allowing for swapping out SQLite for some other database back end?

[u/Acrobatic-Tutor7745 → u/johnpeters42]

Hi, thanks for the feedback. First, I know I should add a readme, I'm just lazy. Currently all it does is serve as a server, it doesn't have a purpose. I also don't know what app I wanna make. Second, I don't mean to sell this code, I mean to build an actual app, that's just a base for something I might expand in the future, not a library. Third, no, I'm not planning to replace SQLite, is there a reason to? From what I know it's fast, and pretty RAM efficient.

[u/johnpeters42 → u/Acrobatic-Tutor7745]

> I'm not planning to replace SQLite, is there a reason to?

Depends what / how much data you're running through it, which will depend on what type of app you end up building on top of this.

[u/Acrobatic-Tutor7745 → u/johnpeters42]

Can you give me a general idea of when to use SQLite compared to other database types?

[u/johnpeters42 → u/Acrobatic-Tutor7745]

Look up SQLite on Wikipedia. There are various features that it leaves out, which is fine if your app doesn't need those features, such as:
* Triggers that automatically run some code when certain database updates occur
* Logins with access to some things in the database but not other things

[u/Acrobatic-Tutor7745 → u/johnpeters42]

It doesn't seem like anything I currently need. Maybe in the future I'll consider changing a database

[u/Tiny_Rent_5936 → u/Acrobatic-Tutor7745]

Apart from the lack of readme, what's exactly the purpose of this, also besides the knowledge You are acquiring doing this. Imo a good starting point should be finding the objective You wanna reach, ie: learn to create a server in cpp, if you're planning to use in an app, what kind of app should be? This is for your requirements only or are to supply other needs. Having this i'm mind, then You could find the actual reason to Cage the app to use Sqlite.