current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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?

share|improve this question
    
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? –  ChrisWue Jan 9 at 10:36
    
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. –  Tomoyose Jan 9 at 10:59
5  
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. –  BlueTrin Jan 9 at 13:51
2  
Meta discussion about voting to close this question –  200_success♦ Jan 9 at 18:49
add comment

2 Answers

up vote 4 down vote accepted

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;
}
share|improve this answer
add comment
up vote 6 down vote

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 ) ;  
}
share|improve this answer
    
Thank you Tomdemuy. All of your suggestions are very useful and will be taken into consideration. –  Tomoyose Jan 9 at 15:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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