Read, calculate, write

Question

This program reads forex files, converts them to the designed timeframe and stores the candles "value" (green or red) in a boolean vector, then it searches for sequence and outputs which it founds and how many times they appeared.

Info about what I did:

At first I tried to make an OOP, but I couldn't do that. I am still not that good. If you have any suggestion on how to transform this program in an OOP manner I would like to hear. I created my own mkgmtime function because there is not in the STL, and the most important thing is that my function doesn't have DST (the program doesn't need that). I am really bad at commenting but I tried my best, tell me if you have any suggestion on how to improve my comments.

I'd like to know how my code is, if there are any bad habits forming, or any error.

// -------------------- <- can I poste the website where you can get the files?
// METATRADE FILE FORMAT :
// YYYY.MM.GG,HH:MM,O.OOOOOO,H.HHHHHH,L.LLLLLL,C.CCCCCC,0\n
// YYYY.MM.GG,HH:MM,O.OOOOOO,H.HH...

// get_time - put_time
#include <iomanip>
// tm - gmtime
#include <ctime>
// copy - sort - transform
#include <algorithm>
// cout - cin
#include <iostream>
// back_inserter - begin - end
#include <iterator>
// string - find - npos - erase - rfind - to_string - stol - stof - substr
#include <string>
// ifstream - ofstream - in - out - close - is_open
#include <fstream>
// vector - vector<bool> - push_back
#include <vector>
// array
#include <array>
// accumulate
#include <numeric>
// make_pair
#include <utility>

// directory_iterator - is_directory - create_directory - is_empty - path
// filename - string - eof
#include <experimental/filesystem>
namespace filesystem = std::experimental::filesystem::v1;

std::time_t mkgmtime(std::tm *const tm);

int main(int argc, char const *argv[]) try {
  filesystem::path user_files_directory{"data"};
  if (!filesystem::is_directory(user_files_directory)) {
    filesystem::create_directory(user_files_directory);
  }

  if (filesystem::is_empty(user_files_directory)) {
    std::string error{"USER FILES DIRECTORY (" + user_files_directory.string() +
                      ") IS EMPTY - FILL IT"};
    throw error;
  }

  // WE DIVIDE THE FILES IN THE FOLDER BY THE SYMBOL, SO WE THEN CAN CALL THEM
  // BY THEIR PAIR
  std::vector<std::vector<filesystem::path>> user_files_paths_by_symbol;
  {
    // SOME SYSTEMS DON'T HAVE ORDERED DIRECTORY THEREFORE WE ORDER IT
    std::vector<filesystem::path> user_files_paths;
    std::copy(
        filesystem::begin(filesystem::directory_iterator(user_files_directory)),
        filesystem::end(filesystem::directory_iterator(user_files_directory)),
        std::back_inserter(user_files_paths));
    std::sort(user_files_paths.begin(), user_files_paths.end());

    std::string symbol{user_files_paths[0].filename().string().substr(7, 6)};
    int start_index{0};
    int index{-1};
    for (auto &file : user_files_paths) {
      index++;
      std::string filename_string{file.filename().string()};

      // THIS PROGRAM WORKS JUST WITH MT TYPE OF DATA
      if (filename_string.find("DAT_MT") == std::string::npos) {
        std::string error{filename_string + " - IS NOT A METATRADE FILE"};
        throw error;
      }

      if (filename_string.find(symbol) == std::string::npos) {
        std::vector<filesystem::path> row(user_files_paths.begin() +
                                              start_index,
                                          user_files_paths.begin() + index);
        user_files_paths_by_symbol.push_back(row);
        symbol = user_files_paths[index].filename().string().substr(7, 6);
        start_index = index;
      }
    }

    std::vector<filesystem::path> row(user_files_paths.begin() + start_index,
                                      user_files_paths.begin() + index + 1);
    user_files_paths_by_symbol.push_back(row);
  }

  // #START#
  for (auto &symbol : user_files_paths_by_symbol) {
    // WHERE WE STORE THE TYPE OF THE CANDLE (GREEN = TRUE - RED = FALSE)
    std::vector<bool> candles;

    for (auto &current_file_path : symbol) {
      std::ifstream input(current_file_path, std::ios::binary);
      if (!input.is_open()) {
        std::string error{"COULDN'T OPEN : " + current_file_path.string()};
        throw error;
      }

      std::tm tm_support = {0};
      const time_t timeframe{86400};
      const time_t rounder{3600};
      const time_t shift{21600}; // 6H
      std::string pending_string;

      // YYYY.MM.GG,HH:MM,
      input >> std::get_time(&tm_support, "%Y.%m.%d,%H:%M,");
      while (!input.eof()) {
        std::time_t datetime =
            ((mkgmtime(&tm_support) + shift) / rounder) * rounder;

        // O.OOOOOO,H.HHHHHH,L.LLLLLL,C.CCCCCC,0\n
        std::getline(input, pending_string);
        float open = std::stof(pending_string);

        // THIS MAY CALL EOF
        input >> std::get_time(&tm_support, "%Y.%m.%d,%H:%M,");

        // WE SKIP ALL THE THE DATE BETWEEN TIMEFRAME BECAUSE WE DON'T NEED THEM
        while ((mkgmtime(&tm_support) + shift) / timeframe ==
                   datetime / timeframe &&
               !input.eof()) {
          std::getline(input, pending_string);

          // THIS MAY CALL EOF
          input >> std::get_time(&tm_support, "%Y.%m.%d,%H:%M,");
        }

        // O.OOOOOO,H.HHHHHH,L.LLLLLL,C.CCCCCC,0\n
        // WE NEED C.CCCCCC - SO WE SEARCH FOR '.' BECAUSE WE CAN'T SEARCH FOR
        // ',' SEE ABOVE
        float close =
            std::stof(pending_string.erase(0, pending_string.rfind('.') - 1));

        if (close - open >= 0.0) {
          candles.push_back(0);
        } else {
          candles.push_back(1);
        }
      }

      input.close();
      std::cout << current_file_path << " - DONE!" << '\n';
    }

    filesystem::path report_directory{"report"};
    if (!filesystem::is_directory(report_directory)) {
      filesystem::create_directory(report_directory);
    }

    std::ofstream output(report_directory /
                         (symbol[0].filename().string().substr(7, 6) + ".csv"));
    if (!output.is_open()) {
      std::string error{"COULDN'T OPEN : " + report_directory.string() +
                        symbol[0].filename().string().substr(7, 6) + ".csv"};
      throw error;
    }

    for (int candles_to_check{2}; candles_to_check < 10; ++candles_to_check) {
      // THE COMBINATION ARE 2 (BECAUSE IT'S 0 OR 1, 2 ELEMENTS) TO THE POWER OF
      // candles_to_check
      int events_size{2};
      for (int i = 1; i < candles_to_check; i++) {
        events_size *= 2;
      }

      // WE WONT TO STORE THE SEQUENCE OF CANDLES THAT OCCURED AND HOW MANY
      // TIMES APPEARED
      std::vector<std::pair<std::string, int>> events(events_size,
                                                      std::make_pair("", 0));

      // WE WONT TO STOP THIS FOR AT THE LAST SEQUENCE AVAILABLE
      for (size_t candles_current_position{0};
           candles_current_position < (candles.size() - (candles_to_check - 1));
           ++candles_current_position) {

        // GET THE SEQUENCE IN STRING
        std::string binary_string{""};
        binary_string = std::accumulate(
            candles.begin() + candles_current_position,
            candles.begin() + candles_current_position + candles_to_check,
            binary_string, [](std::string &sequence, bool new_element) {
              return sequence + std::to_string(new_element);
            });

        // EACH SEQUENCE IS A UNIQUE DECIMAL NUMBER BECAUSE THE SEQUENCES ARE
        // COMPOSED BY 0 AND 1 - SO THEY ARE LIKE BINARY NUMBERS (YOU WILL SEE
        // THAT THEY ARE BINARY NUMBERS INDEED). WE USE THE DICIMAL NUMBER AS
        // INDEX FOR THE VECTOR TO STORE THE SEQUENCE STRING AND THE TIMES THAT
        // APPEARED THAT SEQUENCE
        long num{std::stol(binary_string)}, dec{0}, base{1};
        while (num > 0) {
          long rem = num % 10;
          dec = dec + rem * base;
          base = base * 2;
          num = num / 10;
        }

        events[dec].second++;
        events[dec].first = binary_string;
      }

      std::sort(events.begin(), events.end(), [](auto &left, auto &right) {
        return left.second > right.second;
      });

      // NEED TO FORMAT THE OUTPUT
      std::string events_size_string{std::to_string(events_size)};
      int events_size_leanding_zero{
          static_cast<int>(events_size_string.size())};

      output << "CANDLES  :   " << candles_to_check << '\n';
      for (int index{0}; index < events_size; index++) {

        output << std::setfill('0') << std::setw(events_size_leanding_zero)
               << index << "   :   " << events[index].first << "  ---  "
               << events[index].second << '\n';
      }

      std::cout << candles_to_check << " CANDLES SEQUENCE - DONE!" << '\n';
    }

    std::cout << "CLOSING " << symbol[0].filename().string().substr(7, 6)
              << ".csv" << '\n';
    std::cout << "############################################" << '\n';
    output.close();
  }
  // #END#

  return 0;
} catch (std::string const reason) {
  std::cout << reason << '\n';
}

// FOR MORE INFO ABOUT THIS FUNCTION SEARCH mktime.cpp IN MSVC
std::time_t mkgmtime(std::tm *const tm) {
  int years{tm->tm_year};
  // CHECK TO PREVENT OVERFLOW
  if (years < 70 || years > 155) {
    std::string error{
        "INVALID DATE : " + std::to_string(years) + "." +
        std::to_string(tm->tm_mon) + "." + std::to_string(tm->tm_mday) + " " +
        std::to_string(tm->tm_hour) + ":" + std::to_string(tm->tm_min)};
    throw error;
  }

  // DAYS OF THE MONTHS BEFORE
  int days_per_months[12] = {-1,  30,  58,  89,  119, 150,
                             180, 211, 242, 272, 303, 333};
  int total_days{(years - 70) * 365};
  int month{tm->tm_mon};
  total_days += days_per_months[month]; // SAME AS tm_yday
  total_days += tm->tm_mday;

  // CHECK IF THIS YEAR IS A LEAP YEAR AND IF IT IS AT LEAST MARCH
  if ((((years % 4 == 0) && (years % 100 != 0)) ||
       ((years + 300) % 400 == 0)) &&
      (month > 1))
    ++total_days;

  // CALCULATE LEAP YEARS (1900 - YOUR_DATE) - 17 (LEAP YEARS FROM 1900 -
  // 1970)
  int leap_days{((years - 1) / 4) - ((years - 1) / 100) +
                ((years + 299) / 400) - 17};
  total_days += leap_days;

  std::time_t total_seconds{static_cast<std::time_t>(
      (total_days * 86400) + (tm->tm_hour * 3600) + (tm->tm_min * 60))};
  return (total_seconds);
}

Answer 1

This is quite interesting! I've learned several things from this that I didn't know about, like how to read a candlestick chart. Very neat! I think you have the core of something useful here, but I think it could be improved in a number of ways.

Use Functions

The most important thing that I think you could do to make this code clearer is to break it up into more functions. Your main() function is very long and contains several different things. Generally, you want to take each task that you want to perform and make a separate function for it. (That function may be further broken down into steps which can be made into other functions depending on how complex it is.)

I see the following tasks happening in main():

1. Checking if the user's data directory exists, and if not, creating it
2. Checking if there's any data in the user's data directory
3. Enumerating the files in the user's data directory & sorting them
4. For every file, read the candles, skipping the ones between a certain timeframe
5. Generating a report

Each one of those steps should be a separate function. Some of those should further be broken down into more functions. You even have comments and open new scope in the places where it's logical to make a new function. When you find yourself doing that in your code, you should take whatever's in that scope and put it into a function with a descriptive name.

OOP

It looks to me like you are using a lot of OOP code here. You're using many objects from the standard template library (std::string, std::cin, std::cout, etc.). The data you end up collecting is only booleans, so a vector<bool> in entirely appropriate. You're using range-based loops where appropriate.

If you were doing more processing on the data, like plotting the candlesticks, then it might make sense to make an object to hold the open, close, high, and low, along with a time stamp. But since you're not, there's no need to do that here.

Comments

In my opinion, you have too many comments. You don't need to comment on which functions you're using from each header, for example. If you broke some of the parts with new scope into separate functions, the comments you have above them would be good function-level comments describing the entire function. But for something like the declaration of the candles vector, you don't need to say "Where we store the type of the candle" as that part is obvious. You could use an enum with values named red and green instead of a bool and then you wouldn't need to explain that in a comment, either.

There's no reason your comments should be in all caps. It looks like shouting, or like you're writing FORTRAN.

Also, this comment could be more useful:

// O.OOOOOO,H.HHHHHH,L.LLLLLL,C.CCCCCC,0\n

What do "O", "H", "L", and "C" stand for? For someone who knows what a candlestick chart is, it might be obvious, but for someone trying to understand the code without that knowledge (as I originally was), it doesn't tell us anything. It might be better to write

// Data format: open, high, low, close, 0, \n
// Each value is floating point value with 5 decimal digits of precision

or something along those lines.

Odd Idioms

As mentioned above, I learned some new things reading this code. In particular, I learned that you can put a try immediately after main(), but not inside the main() function. That compiles and works like you'd expect. It is very odd to see, though. I would just put the try() and catch() blocks inside of main(). I didn't check what the return value is when the catch() is invoked, but I would assume it's probably 0. Regardless it's a little confusing and doesn't really perform any better, so I'd stick with the standard way of doing it.

Answer 2

To supplement @user1118321's answer:

Do not throw anything that does not inherit from std::exception

As a matter of convention, all exceptions should inherit from std::exception. std::runtime_error is generally what you would want to use instead of just throwing a string.

Do not use mutable references for read-only code

for (auto &current_file_path : symbol) {

should be:

for (const auto &current_file_path : symbol) {

Prefer to pass arguments by const reference instead of const pointer if null is not an acceptable input:

Passing nullptr to your mkgmtime() would lead to a crash. You can enforce this never happens by using a reference instead.

Also, mind your your const ordering!

A* const means "a non-changing pointer to a mutable A" I suspect you meant `const A*' which is a "mutable pointer to a const A".

std::time_t mkgmtime(std::tm *const tm);

should be:

std::time_t mkgmtime(const std::tm &tm);