Cursor module for a text-editor

Question

A few weeks ago, I had started to write a text-editor in C++ using SFML library. The work is now paused pretty much because I am a lazy freak. So, I thought I might use this passive time to get reviews on what I have done so far.

As of now, I want to get reviews on the cursor module (for displaying a blinking cursor), I wrote for the text-editor. I am posting the code as it was written and it was written without many comments. (I am not sure if I am supposed to add comments before putting it up for review on the site.)

Cursor.h:

#ifndef CURSOR_H_INCLUDED
#define CURSOR_H_INCLUDED

#include<iostream>
#include<SFML/Graphics.hpp>

class Cursor{
    private:
    sf::Color color;
    sf::RectangleShape cursorLine;
    sf::Clock clock;
    int col, line;
    int txtX, txtY;
    int maxCol;
    float thickness, length;
    int blinkPeriod, lastTime;
    bool visible;

    public:
    float getCol();
    float getLine();
    float getLength();
    float getThickness();
    void move(int, int);
    void setPosition(int,int);
    void setDefaultValues();
    Cursor();
    Cursor(float, float, float, float, int, int, int);
    void draw(sf::RenderWindow *);
    void update();
};

#endif // CURSOR_H_INCLUDED

Cursor.cpp:

#include "Cursor.h"

void Cursor::setDefaultValues(){
    col = 1;
    line = 1;
    lastTime = 0;
    thickness = 2;
    length = 20;
    blinkPeriod = 400;
    visible = true;
    color = sf::Color(0,255,0);
}

Cursor::Cursor(){
    setDefaultValues();
}

Cursor::Cursor(float x, float y, float len, float thick, int txX, int txY, int maxCol){
    setDefaultValues();
    this->maxCol = maxCol;
    txtX = txX;
    txtY = txY;
    col = x;
    line = y;
    length = len;
    thickness = thick;
}

float Cursor::getCol(){
    return col;
}

float Cursor::getLine(){
    return line;
}

float Cursor::getLength(){
    return length;
}

float Cursor::getThickness(){
    return thickness;
}

void Cursor::move(int dx, int dy){
    if(dx>0){
        if(col+dx<=maxCol)col += dx;
        else{
            col = 2;
            line += 1;
        }
    }
    else{
        if(col+dx>0) col +=dx;
        else{
            if(line>1){
                line -= 1;
                col = maxCol-1;
            }
        }
    }
    if(line+dy>0)line+= dy;
}

void Cursor::setPosition(int dx, int dy){
    col = dx;
    line= dy;
}

void Cursor::update(){
    cursorLine.setFillColor(color);
    cursorLine.setSize(sf::Vector2f(thickness, length));
    if((clock.getElapsedTime().asMilliseconds()-lastTime)>blinkPeriod){ visible = !visible;
    lastTime = clock.getElapsedTime().asMilliseconds();
    }
    cursorLine.setPosition((col-1)*(length*0.6)+txtX, (line-1)*(length*1.2)+txtY);
}

void Cursor::draw(sf::RenderWindow *screen){
    if(visible) screen->draw(cursorLine);
}

@officialaimm First remember Loki Astari is a Norse God famous for what? His comment can be reduced to "Yet another text editor". — pacmaninbw
– pacmaninbw ♦, Commented Apr 12, 2017 at 13:19

Sebastian Stern · Accepted Answer · 2017-04-18 14:07:30Z

Indentation

I don't know whether this is just a transcription error, or from your real code, but things like:

if((clock.getElapsedTime().asMilliseconds()-lastTime)>blinkPeriod){ visible = !visible;
lastTime = clock.getElapsedTime().asMilliseconds();
}

are really hard to read. It's easy to miss the visible = !visible part. You shouldn't put statements on the same line as the condition of an if-statement.

Type mismatch

Your members for col and line are of type int, but your getters return a float. The same applies for your constructor. That doesn't seem right. Does your text editor actually support floating point coordinates?

Constness

Declare methods that don't change the inner state of an object const. This is the case for your getters. And maybe also for the draw method, depending on what the sf::RenderWindow::draw() method does with its argument.

Member initialization

Your default constructor leaves your Cursor in an undefined state, since neither the maxCol nor the txtXand txtY members are initialized. So drawing or moving such a cursor will not work correctly since you depend on those members.

Also in your second constructor you are first initializing your members with default values using the setDefaults() method and then directly overwrite them. This is uneccessary. You should initialize your variables in the initializer list of the constructor.

Cursor::Cursor(float x, float y, float len, float thick, int txX, int txY, int maxColumn)
    : col(x)
    , line(y)
    , length(len)
    , thickness(thick)
    , txtX(txX)
    , txtY(txY)
    , maxCol(maxColumn)
    , lastTime(0)
    , blinkPeriod(400)
    , visible(true)
    , color(0, 255, 0)
{
}

To avoid duplication of your initialisation you can redirect your constructors and provide the default values there (I don't know what the default values for txtX, txtY and maxCol should be, so I just wrote a question mark).

Cursor::Cursor() 
    : Cursor(1, 1, 20, 2, ?, ?, ?)
{
}

An alternative would be to initialize your members directly in the header file.

sf::Color color {0, 255, 0};
int col = 1;
int line = 1;
int txtX = ?;
int txtY = ?;
int maxCol = ?;
float thickness = 2;
float length = 20;
int blinkPeriod = 400;
int lastTime = 0;
bool visible = true;

I also think it's good to group some of your constructor parameters conceptually into a struct, like x and y (as well as txX and txY) into a point struct. This way you reduce the number of arguments in your constructor and you make their purpose more clear to the reader and user. I guess there's already such a struct in the sf namespace, it not it's rather trivial to implement.

Magic numbers

In your update method you're using 0.6 and 1.2 to manipulate the length of the drawn cursor. Don't use raw numbers in such a case, use named constants (like const float ScaleX or something). This way it's easier to see what they are supposed to do and can be manipulated more easily.

Mauricio Rondon · Accepted Answer · 2017-04-18 05:46:40Z

The names 'txtX' and 'txtY' don't say anything about the domain as 'line', or 'length' do. Someone would need to see how are you using that variable to figure out what it is.

If you want to make the code more readable, make explanatory variables and helper descriptive methods for example

bool fitsInCurrentLine = col+dx<=maxCol;
if( fitsInCurrentLine ) col += dx;
else moveToNextLine();

instead of

if(col+dx<=maxCol)col += dx;
else{
    col = 2;
    line += 1;
}

When I read the last one, I had to figure out "how" you were doing the thing to know "what" you were doing.

Note that I did not write the moveToNextLine method but you get the Idea of what is happening. Considering col = 2; line += 1;. Is it necessary to know that the initial col is 2? or Should I only know that you are moving to the next line if the line wraps?

It reads more like "well written prose" (as Uncle Bob in his book 'CleanCode' says)

You can do similar with the else block.

In the method setPosition, I think the correct name for the parameters is x and y instead of dx or dy given that they are not changes but just absolute values.

This if statement in the update method could be its own method

void Cursor::blink(){
    bool isTimeToChangeState = (clock.getElapsedTime().asMilliseconds()-lastTime)>blinkPeriod;
    if( isTimeToChangeState ){     
        visible = !visible;
        lastTime = clock.getElapsedTime().asMilliseconds();
    }
}

void Cursor::update(){
    cursorLine.setFillColor(color);
    cursorLine.setSize(sf::Vector2f(thickness, length));
    blink();
    cursorLine.setPosition((col-1)*(length*0.6)+txtX, (line-1)*(length*1.2)+txtY);
}

Finally, by reading

cursorLine.setPosition((col-1)*(length*0.6)+txtX, (line-1)*(length*1.2)+txtY);

you can sort of guess what is txtX. Is it the position of the text area? why just don't just put textAreaX and textAreaY, not perfect (I mean, for sure there should be a better name) but at least it improves a little the readability. In the book "clean code" you can find "A name that makes you go into the code to see that it tries to identify, is a bad name"

That's all I would recommend based on the book and experiences I've got as a developer. Happy coding!

Stack Exchange Network

Cursor module for a text-editor

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Cursor module for a text-editor

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions