# Simple line editor

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');
}

system("CLS");

char strBuffer[30], *tempLine = (char*)malloc(sizeof(char)*(strlen(strBuffer) + 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;
break;
case 'z': printf_s("bye bye!\n");
}
}
return 0;
}

• Ariel Ariel, Why do you cast the return value of malloc()? Example: why *lines = (char**)malloc(...instead of *lines = malloc(...? Mar 13 '18 at 12:05
• the header file: conio.h is not portable. Strongly suggest using the standard C library functions rather than the functions 'exposed' via conio.h Mar 14 '18 at 21:09

• 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.

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.

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()