3
\$\begingroup\$

Refactor this code so that I don't have to insert the parameters any time I need to use one of the functions.

I have a class, say julian, for calculating Julian dates of today, a date I insert and the difference between these two Julian dates.

        int julian::calc_julianToday()
        {
            time_t t = time(0);
            struct tm *now = localtime( &t );
                todDay = now->tm_mday;
                todMonth = now->tm_mon+1;
                todYear = now->tm_year+1900;

              int at = (14 - todMonth) / 12;
              int yt = todYear + 4800 - at;
              int mt = todMonth + 12 * at - 3;
              if (todYear > 1582 || (todYear == 1582 && todMonth > 10) || 
    (todYear == 1582 && todMonth == 10 && todDay >= 15))
                return julToday =  todDay + (153 * mt + 2) / 5 + 365 * yt 
    + yt / 4 - yt / 100 + yt / 400 - 32045;
              else
                return julToday = todDay + (153 * mt + 2) / 5 + 365 * yt + yt / 4 - 32083;

        }
        int julian::calc_juliandate(int day, int month, int year)
        {

              int a = (14 - month) / 12;
              int y = year + 4800 - a;
              int m = month + 12 * a - 3;
              if (year > 1582 || (year == 1582 && month > 10) || 
(year == 1582 && month == 10 && day >= 15))
                return julStart = day + (153 * m + 2) / 5 
    + 365 * y + y / 4 - y / 100 + y / 400 - 32045;
              else
                return julStart = day + (153 * m + 2) / 5 + 365 * y + y / 4 - 32083;

        }

        int julian::dates_diff(int day, int month, int year)
        {
            int start = calcJulStartDate(day, month, year);
            int today = calcJulTodayDate();
            differ = today-start;
            return differ;
        }

Please note that all the variable types are declared in a header file. What I would like to do in main() is to be able to use calc_juliandate() without having to call it and pass its attributes each and every time I need it in another function, as in dates_diff(). Any suggestions about coding style or implementing this?

\$\endgroup\$
  • \$\begingroup\$ I'm seriously at a loss as to what exactly you are trying to achieve. Can you post some code showing how you would like to be able to use it? \$\endgroup\$ – ChrisWue Jan 9 '14 at 10:36
  • \$\begingroup\$ I would like to use the result of calc_julianToday() as a variable accessible by other functions without having to create a new variable inside dates_diff() and re-insert the same attributes only to caclulate calc_julianToday(). Ideally calc_julianToday() should be calculated once. Maybe create a new object of the class julian? I don't know. \$\endgroup\$ – Tomoyose Jan 9 '14 at 10:59
  • 5
    \$\begingroup\$ Just use a normal date/time class and convert when needed. Do not try to rebuilt the whole date library in Julian, you will spend alot of time fixing all the bugs you will have. \$\endgroup\$ – BlueTrin Jan 9 '14 at 13:51
  • 2
    \$\begingroup\$ Meta discussion about voting to close this question \$\endgroup\$ – 200_success Jan 9 '14 at 18:49
4
\$\begingroup\$

In dates_diff, you use a variable named differ. Is that an instance variable? It should be local, because it makes no sense to make it part of the object's state. Better yet, just return the difference directly.

int julian::dates_diff(int day, int month, int year)
{
    int start = calcJulStartDate(day, month, year);
    int today = calcJulTodayDate();
    return today - start;
}

However, the more fundamental question is, what does your julian class look like? Is it just a collection of static methods? I think we could do better than that in C++.

#include <ctime>

class Julian {
    /* Default constructor: today's date */
    Julian() {
        // TODO
    }       

    Julian(int year, int month, int day) {
        // TODO
    }

    explicit operator long() const {
        return this->date;
    }

    long operator-(const Julian &other) const {
        return this->date - other.date;
    }

    Julian operator-(long days) const {
        return Julian();
    }

  private:
    long date;
};

Then you can use the code more naturally:

int main() {
    Julian today, newYear(2014, 1, 1);
    std::cout << (today - newYear) << std::endl;
    return 0;
}
\$\endgroup\$
6
\$\begingroup\$

Welcome to Code Review,

I do not know how to solve your problem, however, your code could use polishing.

  • Magical constants; your code is littered with them, use constants instead
  • Undocumented magical constants, I can guess what 365 or 12 is used for, other constants like 32083 could use a comment or could show the calculation of how you got there
  • Naming; todDay, todMonth and todYear are terrible names
  • Naming; at, ty and mt are even worse
  • Casing; be consistent, you mix underscores with casing and even your casing is not consistent ( calc_julianToday <> calc_juliandate )
  • Indenting! Your indenting is all over the place and distracts the reader
  • Repeating / DRY;

I do not write C++ but can you not write calc_julianToday() as

int julian::calc_julianToday()
{
  time_t t = time(0);
  struct tm *now = localtime( &t );
  return julToday =  calc_juliandate( now->tm_mday, now->tm_mon+1, now->tm_year+1900 ) ;  
}
\$\endgroup\$
  • \$\begingroup\$ Thank you Tomdemuy. All of your suggestions are very useful and will be taken into consideration. \$\endgroup\$ – Tomoyose Jan 9 '14 at 15:27

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.