patterncppMinor
Line and column tracker for text files
Viewed 0 times
linecolumntexttrackerfilesforand
Problem
A class that keeps track of lines and columns in a text file. I use it in a parser so that when an error occurs, I can print out the line and column at which it occured.
Here's line col.hpp
Here's line col.cpp
```
#include "line col.hpp"
LineCol::LineCol(LineType line, ColType col)
: line(line), col(col) {
assert(line >= FIRST_LINE);
assert(col >= FIRST_COL);
}
void LineCol::update(char c) {
switch (c) {
case '\t':
col += SIZE_OF_TAB;
break;
case '\n':
line++;
col = FIRST_COL;
break;
//vertical tab and form feed do the same thing on my system
case '\v':
case '\f':
line++;
break;
case '\r':
col = FIRST_COL;
break;
case '\b':
//backspace doesn't move up lines
//when backspacing tabs it treats them as spaces so it only
//moves back one char
if (col != FIRST_COL) {
col--;
}
//i don't have to put a break here but should I even thou
Here's line col.hpp
#ifndef line_col_hpp
#define line_col_hpp
#include
#include
#include
///Keeps track of lines and columns in text.
///Great for writing error messages in parsers
class LineCol {
public:
using LineType = unsigned int;
using ColType = unsigned int;
static const ColType SIZE_OF_TAB = 8;
static const LineType FIRST_LINE = 1;
static const ColType FIRST_COL = 1;
explicit LineCol(LineType line = FIRST_LINE,
ColType col = FIRST_COL);
///Move line and col according to the char.
///Call this at the end of the loop with the char you just processed
void update(char);
///This calls update(char) for each char in the null terminated string
void update(const char *);
///This calls update(char) for the first n chars in the string
void update(const char *, size_t);
///Sets line to FIRST_LINE and col to FIRST_COL
void reset();
LineType getLine() const;
ColType getCol() const;
const char *getStr() const;
private:
LineType line;
ColType col;
};
#endifHere's line col.cpp
```
#include "line col.hpp"
LineCol::LineCol(LineType line, ColType col)
: line(line), col(col) {
assert(line >= FIRST_LINE);
assert(col >= FIRST_COL);
}
void LineCol::update(char c) {
switch (c) {
case '\t':
col += SIZE_OF_TAB;
break;
case '\n':
line++;
col = FIRST_COL;
break;
//vertical tab and form feed do the same thing on my system
case '\v':
case '\f':
line++;
break;
case '\r':
col = FIRST_COL;
break;
case '\b':
//backspace doesn't move up lines
//when backspacing tabs it treats them as spaces so it only
//moves back one char
if (col != FIRST_COL) {
col--;
}
//i don't have to put a break here but should I even thou
Solution
Overall I'd say this code is very easy to read. I can see quickly how everything works. Here are a few suggestions:
Only Make Public What's Necessary
There are 3
I'd also make the argument to
Naming
Name Your Method Arguments!
What do the method arguments represent? Presumably for the 2
Improve Your Method Names
The method name
The
Even
Fix This Bug
In
All of this could be avoided if you...
Use
Since you're using C++, you should be using C++'s
Only Make Public What's Necessary
There are 3
update() functions. Looking at them, it's not clear what the purpose of them is. The first one doesn't look like it should be public at all. It changes the internal state of the object, but doesn't update the string it comes from, which is confusing. It means that a caller could repeatedly call it with the same character from a string and it would count incorrectly. I'd make that version private.I'd also make the argument to
void update(char) be const since it's not modified.Naming
Name Your Method Arguments!
What do the method arguments represent? Presumably for the 2
update() methods that take a pointer to a character, they represent a string to be parsed. They should have a name like parseString or something similar.Improve Your Method Names
The method name
update() doesn't tell me anything about what the function does. Any method that modifies the state of an object updates it in some way. A better name might be parse() or advanceLineAndColumn() or something that says what it actually does.The
getStr() method is very deceptive because it looks like it gets a pointer to the string this object is parsing. But it doesn't! Since this is C++, you'd be better served by making an operator<<() method or free function to print out your object.Even
reset() could be a little more descriptive. Perhaps something like resetParsing() or resetLineAndColumn(). Consequently, why would I want to use reset() instead of just creating a new instance? They seem fairly lightweight, so what's the advantage? If there is one, maybe that would be helpful in renaming the method.Fix This Bug
In
updated(str, size); you have a potential memory stomper. If size is greater than the actual length of str, you'll call update(str); and str might point to memory you don't have permission to read, causing a crash. Alternatively it might point to other data in your app and cause other problems such as incorrect line and column values.All of this could be avoided if you...
Use
std::stringSince you're using C++, you should be using C++'s
string class to handle strings. It eliminates the possibility of running off the end of the string because you can access it using the at() method, which will throw an exception if you try to access a character outside the length of the string.Context
StackExchange Code Review Q#156328, answer score: 5
Revisions (0)
No revisions yet.