2
\$\begingroup\$

I'm self-learning C now and I want to find out if my code is good and clean or kinda messy. This exercise is a simple line editor. please tell me what do you think, If anything needs to be better in the terms of style and bugs. for the program purpose itself, I don't intend to change or add features to it. please tell me what you think.

#include <stdio.h>
#include <stdlib.h>
#include <conio.h>
#include <string.h>
#include <malloc.h>


char *_ctrlLine = "#################################################";
int _cursor = 0, _maxLines = 0;


void clearBuffer(void) {
    while (getchar() != '\n');
}

void addLine(char ***lines) {
    system("CLS");

    char strBuffer[30], *tempLine = (char*)malloc(sizeof(char)*(strlen(strBuffer) + 1));
    printf("enter your row:\n>%3d ", _cursor+1);
    gets_s(strBuffer, 30);
    strcpy_s(tempLine, strlen(strBuffer) + 1, strBuffer);

    _maxLines++;
    char **temp = *lines;
    *lines = (char**)malloc(sizeof(char*) * (_maxLines+2));

    for (int i = 0; i < _cursor+1; i++)
        (*lines)[i] = temp[i];
    (*lines)[_cursor] = tempLine;
    for (int i = _cursor; i <= _maxLines; i++)
        (*lines)[i+1] = temp[i];
    free(temp);

}

void deleteLine(char ***lines) {
    system("CLS");

    if (_maxLines == 0 || _cursor == _maxLines) {
        printf((_maxLines == 0) ?
            "no lines to delete.. YET!\n" :
            "you cant delete the control line.\n");
        getchar();
        clearBuffer();
        return;
    }

    _maxLines--;
    char **temp = *lines;
    *lines = (char**)malloc(sizeof(char*) * (_maxLines+1));
    for (int i = 0; i < _cursor; i++)
        (*lines)[i] = temp[i];
    for (int i = _cursor; i <= _maxLines; i++)
        (*lines)[i] = temp[i+1];
    free(temp[_cursor]);
    free(temp);
}

void changeLine(char ***lines) {
    system("CLS");

    if (_maxLines == 0 || _cursor == _maxLines) {
        printf((_maxLines == 0) ?
            "no lines to change.. YET!\n" :
            "you cant change the control line.\n");
        getchar();
        clearBuffer();
        return;
    }

    char *temp = (*lines)[_cursor];
    puts(temp);
    printf("change the row above:\n>%3d ", _cursor+1);
    char strBuffer[30], *tempLine = (char*)malloc(sizeof(char)*(strlen(strBuffer) + 1));
    gets_s(strBuffer, 30);
    strcpy_s(tempLine, strlen(strBuffer) + 1, strBuffer);
    (*lines)[_cursor] = tempLine;
    free(temp);
}

void printLines(char **lines) {
    system("CLS");

    if (_maxLines == 0) {
        printf_s("no lines... YET!\n");
        return;
    }
    for (int i = 0; i <= _maxLines; i++) {
        putchar((_cursor == i) ? '>' : ' ');
        printf_s(" %3d %s\n", i+1, lines[i]);
    }
}

void moveCursor(bool up) {
    if (up) {
        if (_cursor > 0)
            _cursor--;
    }
    else if (_cursor < _maxLines)
        _cursor++;
}

void showOptions() {
    system("CLS");
    printf_s("a. add line\nd. delete line\nc. change line\n1. move cursor up\nq. move cursor down\nz. quit\npress any key to continue ");
    getchar();
    clearBuffer();
}

int main() {
    char key = NULL;
    char **lines = (char**)malloc(sizeof(char*));
    lines[0] = _ctrlLine;

    while (key != 'z') {
        printLines(lines);
        printf_s("what key do you want?(o for options) ");
        scanf_s("%c", &key);
        clearBuffer();
        switch (key) {
        case 'o': showOptions();
            break;
        case '1': moveCursor(1);
            break;
        case 'q': moveCursor(0);
            break;
        case 'd': deleteLine(&lines);
            break;
        case 'c': changeLine(&lines);
            break;
        case 'a': addLine(&lines);
            break;
        case 'z': printf_s("bye bye!\n");
        }
    }
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Ariel Ariel, Why do you cast the return value of malloc()? Example: why *lines = (char**)malloc(...instead of *lines = malloc(...? \$\endgroup\$ – chux - Reinstate Monica Mar 13 '18 at 12:05
  • \$\begingroup\$ the header file: conio.h is not portable. Strongly suggest using the standard C library functions rather than the functions 'exposed' via conio.h \$\endgroup\$ – user3629249 Mar 14 '18 at 21:09
4
\$\begingroup\$
  • malloc doesn't need a cast. In fact, casting the result of malloc may lead to hard to find problems.

  • sizeof(char) is guaranteed to be 1.

  • Prefer taking sizeof expression rather than sizeof(type). The reason is that such code remains valid even if the type changes. In your case,

        *lines = malloc(sizeof(**lines) * (_maxLines+2));
    
  • Triple stars are usually frowned upon. Notice that you don't even need them. Your code never refers to lines per se, but only to *lines. This is what you care about, and that is what you shall be passing.

  • lines, cursor and maxLines are good candidates to be bundled into a structure.

  • The strBuffer serves no purpose. You may read directly into tempLine. Similarly, changeLine may read directly into (*lines)[_cursor], avoiding memory allocation.

  • deleteLine doesn't really need temp. You may modify lines in-place.

  • A printf(); getchar(); clearBuffer() sequence should be factored into a function.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Most would prefer something like [i+1] to be [i + 1]. As a general rule make sure your operators have a space each side. But since you are a bit inconsistent with the spacing it may just be a typo.

Not including { and } with your for loops is playing with fire and something I would discourage.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

for ease of understanding and readability:

  1. separate code blocks ( for if else while do...while switch case default ) via a single blank line.
  2. separate functions by 2 or 3 blank lines (be consistent)
  3. follow the axiom: only one statement per line and (at most) one variable declaration per statement.

============================ in general, it is best to layout the code in the sequence:

  1. the prototypes for the sub functions
  2. the 'main()' function
  3. the bodies of the sub functions

============================ in general, variable names should not begin with _<capital letter> nor with __ as such naming conventions are 'reserved' for the system Amongst other reasons, the compiler prepends all names with a '_'

============================ the header file: stdlib.h exposes the prototype for malloc() so no need to include the malloc.h header file

============================ regarding:

while (getchar() != '\n'); 

this is not sufficient as the EOF condition can be returned so should also be checking for EOF

============================ regarding:

system("CLS");

the 'command' "CLS" is not portable. Suggest using a formfeed character (or better) use the ANSI terminal escape sequence for clearing the screen and moving the cursor to the 0,0 (upper left) char position

============================ regarding:

void addLine(char ***lines)

please google 'three star programmer' then rethink the logic of that part of the program.

============================ the function: gets_s() is not portable, suggest using: fgets()

============================ when calling any of the heap allocation functions: ( malloc calloc realloc )

  1. the returned type is void* which can be assigned to any pointer. Casting just clutters the code, making it more difficult to understand, debug, etc and is 'error prone' when performing debug and/or maintenance.
  2. the expression: sizeof(char) is defined in the C standard as 1. multiplying anything by 1 has no effect and just clutters the code.
  3. always check (!=NULL) the returned value to assure the operation was successful.

============================ the function: strcpy_s() is not portable, suggest using: strncpy()

============================ when writing the body of a for() or while(), etc loop, even if there is only (today) a single statement, always enclose the body in braces '{' '}' so some future (or yourself) will not be likely to corrupt the logic flow

============================ regarding:

char strBuffer[30]
char *tempLine = malloc(strlen(strBuffer) + 1));

since the array strBuffer contains trash and since the function: strlen() does not stop until it encounters a NUL byte (0x00), the result of the call to strlen() could return anything (depending on where it encountered a NUL byte) This results in undefined behavior and can lead to a seg fault event.

Note: several of the places where the posted code is calling strlen(), from the surrounding code, it should be using: sizeof()

============================ there are several problems with the handling of the variables: cursor temp *lines and maxLines amongst other things, this statement:

*lines = malloc(maxLines+2);

is only allocating a 'trivial' number of bytes. BUT *lines is expected to be an array of pointers to char. But that is not what is being allocated.

============================ regarding:

free(temp);

is hinting that ALL the pointers to the lines in the file are being copied each time a line is inserted into the file.

============================ Note: variable names should indicate content or usage (or better, both) Several of the variable names are very misleading.

============================ in the function: addline(), there is a LOT of copying as a line is inserted in the text. IMO: it would be much better to simply extend the sizeof the *lines memory allocation via calling realloc()

| improve this answer | |
\$\endgroup\$

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.