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\$
4
  • \$\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
    Commented Jan 9, 2014 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
    Commented Jan 9, 2014 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
    Commented Jan 9, 2014 at 13:51
  • 2
    \$\begingroup\$ Meta discussion about voting to close this question \$\endgroup\$ Commented Jan 9, 2014 at 18:49

2 Answers 2

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\$
1
  • \$\begingroup\$ Thank you Tomdemuy. All of your suggestions are very useful and will be taken into consideration. \$\endgroup\$
    – Tomoyose
    Commented Jan 9, 2014 at 15:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.