cli handler pr #29

BAOELIETRAN · 2024-06-19T04:02:48Z

Pull request name goes here

What:

Handling the prompt from user

Whenever user run the file with a random argument, a prompt will pop up.
Declare new function smoldb_input_buf_read to put the content of the user's input into the buffer of the InputBuf struct, and also set the length of the buffer.
Declare new function prompt_prototype to receive the check the argument, read the user's input, and handle the request.
Why:

Declare new functions for the prototype of the prompt.

How:

Declare new functions, and handle the edge cases of the prompt, such as if user type "mogging", the program will exit.

Misc:

I think we should discuss how to make the prompt more colorful and interesting, like moving figure or sth?

Tags:

@huynguyen-and-friend-projects/everyone

Pull request by: Bao Ngo

commit-by: Bao Ngo

…nto beta-v0.0.1 pull

nguyenhuy0905 · 2024-06-19T04:16:51Z

Here's a random trick to create colorful prompt: use ANSI escape codes. Imma just "steal" my old code and paste it here:

#define RESET_ALL "\033[0m"

/* List of colors */

#define BLACK "\033[30m"
#define RED "\033[31m"
#define GREEN "\033[32m"
#define YELLOW "\033[33m"
#define BLUE "\033[34m"
#define MAGNETA "\033[35m"
#define CYAN "\033[36m"
#define WHITE "\033[37m"
#define DEFAULT_COLOR "\033[39m"

/* List of formats */

#define BOLD "\033[1m"
#define DIM "\033[2m"
#define ITALIC "\033[3m"
#define UNDERLINE "\033[4m"
#define STRIKETHROUGH "\033[9m"

How you would use these is like so:
Say you want to print "Hello World" in red,

printf("\033[31m" "Hello World\n" "\033[0m");

or, if you paste all the #defines above into your source code:

printf(RED "Hello World\n" RESET_ALL);

The "\033[0m" (aka, RESET_ALL in this case) is important, otherwise, the next printf will still show red even though you didn't specify a color.

EDIT: sometimes you see people use \x1b instead of \033 as the prefix. It will yield the same result, so far as I'm aware, anyways.

src/cli/cli-handler.c

src/cli/cli-handler.h

src/CMakeLists.txt

nguyenhuy0905 · 2024-06-19T16:04:17Z

Another trick, here's how you would make a progress bar:

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
    uint progress = 0;
    while(progress <= 100){
        printf("\r%d%%  ", progress);
        for(int i = 0; i <= progress; i += 10){
            printf("#");
        }
        progress += 10;
        sleep(1);
        fflush(stdout);
    }
    printf("\n");
    
    return EXIT_SUCCESS;
}

The output is something of:

30% ###

On Windows machine, change the unistd include to windows, and change sleep to Sleep.
Although, I dunno what we may be using a progress bar for.

change the color of the text of the prompt.

nguyenhuy0905 · 2024-06-21T05:37:23Z

src/cli/cli-handler.h

@@ -39,8 +39,26 @@ SMOL_API int smoldb_new_input_buf(InputBuf **buf);
 */
 SMOL_API int smoldb_free_input_buf(InputBuf **buf);

+/**
+ * @brief Prototype to handle prompt, it will get what the user type in and change it into InputBuf
+ * @param InputBuf* buf is a pointer point to the InputBuf, argc is the number of arguments, and args is


Nitpick; we all know buf is a pointer pointing to the InputBuf. So, that part is very much unnecessary and you can delete it. Also, declare each variable in its own @param tag. And, put the looooong description either at the bottom or right below the @brief tag. So, something like this (example stolen from this site):

/** @brief Prints the string s, starting at the current * location of the cursor. * * If the string is longer than the current line, the * string should fill up the current line and then * continue on the next line. If the string exceeds * available space on the entire console, the screen * should scroll up one line, and then the string should * continue on the new line. If '\n', '\r', and '\b' are * encountered within the string, they should be handled * as per putbyte. If len is not a positive integer or s * is null, the function has no effect. * * @param s The string to be printed. * @param len The length of the string s. * @return Void. */ void putbytes(const char* s, int len);

I wouldn't consider this an issue though, this is just a nitpick.

nguyenhuy0905 · 2024-06-21T05:38:48Z

src/cli/cli-handler.h

+ * In this function, my main purpose is that whenever the user insert a new "input" string, the content
+ * of the string will be assigned to the string buffer in InputBuf, and the length of the string will also 
+ * be assigned to buf_len.
+ * Inside the smoldb_input_buf_read function, I reallocate the memory space for InputBuf with the lenght of 


Nitpick, don't use a first-person word. Again, it's a nitpick, I wouldn't really consider it an issue.

nguyenhuy0905 · 2024-06-21T05:40:02Z

src/cli/cli-handler.c


 #include "retval.h"

+// I will define some styling here:


Don't fill the codebase with these explanation. In this case, it's obvious that styling options are being defined.

nguyenhuy0905 · 2024-06-21T05:50:09Z

src/cli/cli-handler.c

+    perror(RED BOLD "Point to NULL\n" RESET_ALL);
+    return SMOLDB_NULL_PTR_TO_REF_ERR;
+  };
+  // want to make animatic greetings.


I'm afraid it's not very possible, at least without some APIs like ncurses. If you're printing the animation, your cursor is at the position of the animated text, meaning the user cannot input their prompt(s).

Maybe ncurses provides support for such tasks, but it's yet another thing to learn, just for a small trick, so maybe we can add these cherries-on-top after we have finished the basic functionalities of our database.

Also, if you have any questions, you should open an issue or start a discussion. That way everyone sees that and (may) help you.

nguyenhuy0905 · 2024-06-21T05:53:35Z

src/cli/cli-handler.c

+  //   // Move the cursor to the beginning of the line
+  //   printf("\r");
+  // }
+  printf(GREEN BOLD "Welcome to Mogwarts university\n" RESET_ALL);


You should leave out these eccentric prints. These are fun at the beginning, but may become hell for bug-tracking later on, when the codebase grows.

prompt or just do some whitespace input, the program will reprompt the user.

nguyenhuy0905

Still no approval yet 😄. My main issue is with usability. The wait time between running the program and waiting the animation to finish is pretty damn long.
I heard this recently. No programming project is uncool, but you need to put yourself into the shoes of the users. Make it practical; and wrap the product in great packaging. So, the part on the animation, I will let you decide what to do with that. Sorry if I'm too strict 😄.
There are some minor bugs I will help you fix here. Well, mainly the memory leak due to the exit statements.
Still, I think the animation is fun. Were you to remove the animation, I would instead suggest you branch out and cherry-pick the changes instead.

nguyenhuy0905 · 2024-06-24T03:27:59Z

src/cli/cli-handler.c

+      smoldb_input_buf_read(buf, prompt_input);
+      if (strcmp(prompt_input, "mogging") == 0){
+        printf(YELLOW BOLD "Bro can rizz now!\n" RESET_ALL);
+        exit(0);


In case you're unaware, these exit statements cause some memory leak. Simply removing them will fix it, because by doing that, the free function in main() is called.

nguyenhuy0905 · 2024-06-24T03:28:23Z

src/cli/cli-handler.c

+    }
+  }
+  printf(BOLD "\nBro is NOT Jordan Barrett\n" RESET_ALL);
+  exit(1);


Same here. But, I will remove them for you. It's a simple one.

nguyenhuy0905 · 2024-06-24T03:30:06Z

src/CMakeLists.txt

@@ -9,6 +9,7 @@ set(ENTRY_SRC entry.c)

 add_executable(smoldb ${ENTRY_SRC})
 target_link_libraries(smoldb smoldb-cli smoldb-internal)
+target_compile_options(smoldb PRIVATE "-pthread")


Does the program still use pthread? If not, I can remove this option here.

nguyenhuy0905 · 2024-06-24T03:30:40Z

src/cli/CMakeLists.txt


 add_library(smoldb-cli STATIC ${CLI_SRC})
 target_link_directories(smoldb-cli INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
 target_link_libraries(smoldb-cli PUBLIC compile-opts)
+if(Threads)
+    target_compile_options(smoldb-cli PRIVATE "-pthread")


Same here, if there's no pthread used, I can remove this.

nguyenhuy0905 · 2024-06-24T04:07:49Z

It's quite disheartening to say this, and maybe I'm too strict here, but the animation, while cool, doesn't enhance user experience, but kinda hammers it, due to the loading time. 😞

Also, print functions are considered very "expensive" on the processor, due to it being a syscall.

If you're a recruiter, you would prefer more practical tools, right? 😄

There are some minor issues like memory leaks, which I can fix for you. But the big ones, like animation, is up to your decision. Were it me, I would save the changes on a different branch first, then change this branch's code to be more pragmatic. That way, you won't lose the cool stuff.

Lastly, I think we shall remove the "explanatory" comments? These ones:

I suggest that, the parts of the code you don't understand, you should copy that part of the code into a, say, text file. I prefer Markdown because you can get syntax-highlighting like this:

std::print("Isn't it cool that we get some syntax highlights?");
while(true){
  SomeClassToCauseMemLeak* evil = new SomeClassToCauseMemLeak;
}

Then, write a detailed explanation below the code snippet.

Anyways, really nice effort!

nguyenhuy0905

I suggest cherry-picking the btree.cpp file into a different branch, then remove the btree.cpp file from this branch, then make a PR on the new branch. That way, each PR is more focused.

nguyenhuy0905 · 2024-07-07T03:10:54Z

src/internal/btree.cpp

@@ -0,0 +1,515 @@
+#include <iostream>
+using namespace std;


Please don't using namespace std;, since that kind of makes namespaces useless.
I think they usually do something like this for very long namespaces:

namespace some_obfuscatingly_extravagantly_long_namespace = soeln;

But to be honest, is std really a "long" one?

I understand

nguyenhuy0905 · 2024-07-07T03:23:34Z

src/internal/btree.cpp

+    // minimum degree (defines the range for number of keys)
+    int t;
+    // An array of child pointers
+    BTreeNode* *CP_arr;


I would say, the "better" way is to declare the class as a template, and CP_arr as your everyday std::array. So, something like this (note: possibly wrong syntax. I don't really know C++):

template <std::size_t min_deg> class BTreeNode { private: std::array<int, 2*min_deg + 1> keys; std::array<std::unique_ptr<BTreeNode>, 2*min_deg> children; std::size_t n_keys; ... };

nguyenhuy0905 · 2024-07-07T03:24:42Z

src/internal/btree.cpp

+    bool leaf;
+public:
+    // Constructor:
+    BTreeNode(int min_degree, bool isleaf); 


I don't know if it's even necessary, but you may include also a destructor, a copy constructor, a move constructor, a copy assignment, and a move assignment.
Also, declare this constructor as explicit. Otherwise, this class still has an implicit default constructor that takes no value.

nguyenhuy0905 · 2024-07-07T03:27:11Z

src/internal/btree.cpp

+
+    // Insert a new key in the subtree rooted with this node
+    // The node must bbe non-full when this function is called
+    void insertNonFull(int new_key);


I think, since you require the node to be non-full, it is better to make this function private. Expose a public function that first checks if the node called is non-full, and if it is, the private function is called.
We all love writing docs to warn other coders, but, you know, we are all lazy. 😄

nguyenhuy0905 · 2024-07-07T03:28:40Z

src/internal/btree.cpp

+    // array of key
+    int *keys;
+    // minimum degree (defines the range for number of keys)
+    int t;


You shouldn't name variables with 1 letter like so, unless it's already a convention (eg, size_t i for a for loop).

nguyenhuy0905 · 2024-07-07T03:38:56Z

src/internal/btree.cpp

+    // Allocate memory for maximum number of possible keys
+    // and child pointers
+    keys = new int[2*min_degree - 1];
+    CP_arr = new BTreeNode*[2*min_degree];


Also, since I have yet to see the BTreeNode's destructor (that deletes these 2 elements you call new on), this causes memory leak.

nguyenhuy0905 · 2024-07-07T03:40:23Z

src/internal/btree.cpp

+
+    // Freeing the memory occupied by sibling
+    delete(sibling);
+    return;


return here is not necessary. Though, it doesn't affect the code by one bit, so you're still good.

nguyenhuy0905 · 2024-07-07T03:44:13Z

src/internal/btree.cpp

+}
+
+// Function to traverse all nodes in a subtree rooted with this node
+void BTreeNode::traverse(){


You should put all that is above this function in a header file, and all below this, keep it in this file. Otherwise, no other source files can call these functions.

I think I already put them in the header file

nguyenhuy0905 · 2024-07-07T03:50:30Z

src/internal/btree.cpp

+        delete tmp;
+    }
+    return;
+}


You forgot to free the memory held by each BTreeNodes after the BTree goes out of scope or is deleted. To do that, since you're manually calling new, you need to implement some form of tree traversal (DFS, BFS) to reach all BTreeNodes, then free memory inside each node (if you call new for any of the memory inside that).

Oh yea, you don't have to do a DFS or BFS. You just need to create a destructor for BTreeNode, and inside it, call delete on all the things inside the BTreeNode you allocated with new, and do the same with the BTree.

nguyenhuy0905 · 2024-07-07T03:51:37Z

src/internal/btree.cpp

+
+    // A function to borrow a key from the CP_arr[idx + 1] -th 
+    // node and place it in C[idx]th node (mượn thằng anh em bên phải)
+    void borrowFromNext(int idx);


I don't think you should expose the two borrow functions to public. You don't want the user to call it after all.

nguyenhuy0905 · 2024-07-07T14:37:58Z

src/internal/btree.cpp

+BTreeNode* BTreeNode::search(int key){
+    int index = 0;
+    while (index < num_keys && keys[index] < key){
+        index ++;


You should do a binary search here, since the array of keys is always sorted, iirc.

good advice, I will take that when implementing new btree

nguyenhuy0905 · 2024-07-12T22:33:57Z

@BAOELIETRAN also, check my implementation of BTree. I got it working (halfway) already. Thanks a lot for your motivation to write this hell of a data structure. Your implementation (as well as mine) is of course, not perfect, but still, that gave me the motivation necessary. It took me a few days and 2 rewrites just to get search() and insert() on mine work properly.

I just force-push my BTree implementation onto a separate repository in this organization lol.

Here's the link to my BTree implementation

BAOELIETRAN

good

src/cli/cli-handler.c

src/cli/cli-handler.h

BAOELIETRAN · 2024-06-24T01:51:30Z

src/cli/cli-handler.c


 #include "retval.h"

+// I will define some styling here:


BAOELIETRAN · 2024-06-24T03:53:51Z

src/cli/cli-handler.c

+      smoldb_input_buf_read(buf, prompt_input);
+      if (strcmp(prompt_input, "mogging") == 0){
+        printf(YELLOW BOLD "Bro can rizz now!\n" RESET_ALL);
+        exit(0);


BAOELIETRAN · 2024-07-14T01:55:39Z

src/cli/cli-handler.h

@@ -39,8 +39,26 @@ SMOL_API int smoldb_new_input_buf(InputBuf **buf);
 */
 SMOL_API int smoldb_free_input_buf(InputBuf **buf);

+/**
+ * @brief Prototype to handle prompt, it will get what the user type in and change it into InputBuf
+ * @param InputBuf* buf is a pointer point to the InputBuf, argc is the number of arguments, and args is


BAOELIETRAN · 2024-07-14T01:58:58Z

src/cli/cli-handler.h

+ * In this function, my main purpose is that whenever the user insert a new "input" string, the content
+ * of the string will be assigned to the string buffer in InputBuf, and the length of the string will also 
+ * be assigned to buf_len.
+ * Inside the smoldb_input_buf_read function, I reallocate the memory space for InputBuf with the lenght of 


nguyenhuy0905 · 2024-07-14T03:55:52Z

@BAOELIETRAN in case you're still working on this PR. You have a merge conflict here, unfortunately 😞. Before you proceed, you should resolve the merge conflicts first.

Ngo Gia Bao added 3 commits June 18, 2024 22:47

feat: start doing cli handler

d964b96

commit-by: Bao Ngo

Merge branch 'beta-v0.0.1' of https://github.com/BAOELIETRAN/smoldb i…

62db609

…nto beta-v0.0.1 pull

feat: fix cli-handler.c

f315b33

nguyenhuy0905 reviewed Jun 19, 2024

View reviewed changes

src/cli/cli-handler.c Outdated Show resolved Hide resolved

src/cli/cli-handler.c Outdated Show resolved Hide resolved

src/cli/cli-handler.h Outdated Show resolved Hide resolved

src/cli/cli-handler.h Outdated Show resolved Hide resolved

src/CMakeLists.txt Show resolved Hide resolved

nguyenhuy0905 assigned BAOELIETRAN and nguyenhuy0905 Jun 19, 2024

nguyenhuy0905 linked an issue Jun 19, 2024 that may be closed by this pull request

Add a simple REPL #13

Open

feat: I just write some more comments for the functions I wrote, and I

4dbd4ba

change the color of the text of the prompt.

nguyenhuy0905 reviewed Jun 21, 2024

View reviewed changes

feat: I just fixed the prompt so that everytime an user insert the empty

f0422f8

prompt or just do some whitespace input, the program will reprompt the user.

nguyenhuy0905 reviewed Jun 24, 2024

View reviewed changes

nguyenhuy0905 mentioned this pull request Jun 24, 2024

Pr29 help BAOELIETRAN/smoldb#1

Closed

Ngo Gia Bao added 2 commits June 24, 2024 01:01

commit

cd5266d

commit

0f7a9cf

nguyenhuy0905 reviewed Jul 7, 2024

View reviewed changes

BAOELIETRAN commented Jul 14, 2024

View reviewed changes

cli handler pr #29

Are you sure you want to change the base?

cli handler pr #29

Conversation

BAOELIETRAN commented Jun 19, 2024 • edited by nguyenhuy0905 Loading

Pull request name goes here

nguyenhuy0905 commented Jun 19, 2024 • edited Loading

nguyenhuy0905 commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy0905 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy0905 commented Jun 24, 2024

nguyenhuy0905 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy0905 Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy0905 commented Jul 12, 2024 • edited Loading

BAOELIETRAN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy0905 commented Jul 14, 2024

BAOELIETRAN commented Jun 19, 2024 •

edited by nguyenhuy0905

Loading

nguyenhuy0905 commented Jun 19, 2024 •

edited

Loading

nguyenhuy0905 commented Jun 19, 2024 •

edited

Loading

nguyenhuy0905 Jul 12, 2024 •

edited

Loading

nguyenhuy0905 commented Jul 12, 2024 •

edited

Loading