Which piece of code is better?

Question

I am was writing some code, but I´m not sure which is better. In one way it's easier read what is happening, but I have more line of code. In the other way, you has less line of code but I think is harder to understand.

String imp = importance.getSelectedItem().toString();
String title_str = title.getText().toString();
String body_str = body.getText().toString();
String location_str = location.getText().toString();
int day = date.getDayOfMonth();
int month = date.getMonth()+1;
int year = date.getYear();
int hh = time.getCurrentHour();
int mm = time.getCurrentMinute();
String date_str = year+"/"+month+"/"+day+" " + hh+":"+mm +":00"; // yyyy/MM/dd HH:mm:ss
long dateMilliseconds = new Timeconversion().timeConversion(date_str);

Conference conference = ConferenceBuilder.conference()
        .id(idConf)
        .importance(Double.parseDouble(imp))
        .title(title_str)
        .body(body_str)
        .location(location_str)
        .timeInMilliseconds(dateMilliseconds)
        .build();

or

Conference conference2 = ConferenceBuilder.conference()
                                .id(idConf)
                                .importance(Double.parseDouble(importance.getSelectedItem().toString()))
                                .title(title.getText().toString())
                                .body(body.getText().toString())
                                .location(location.getText().toString())
                                // yyyy/MM/dd HH:mm:ss
                                .timeInMilliseconds(new Timeconversion().timeConversion(date.getYear()+"/"+date.getMonth()+1+"/"+date.getDayOfMonth()+" " + time.getCurrentHour()+":"+time.getCurrentMinute() +":00"))
                                .build();

Go for readability. And by the way, the word is "piece", not "peace". "Peace" means not fighting. — Mike Dunlavey, Oct 27 '16 at 22:01
Also I think this is better served as a java question. I think there's nothing relevant to Android or that requires knowledge of Android here. — Chantell Osejo, Oct 27 '16 at 22:36
Although it is an android code I think you're right, I will remove it. Thanks — mavi, Oct 27 '16 at 22:50

Chantell Osejo · Answer 1 · 2016-10-27T23:09:04.950

1

Split the difference. I'd do something like this:

Conference conference2 = ConferenceBuilder.conference()
            .id(idConf)
            .importance(Double.parseDouble(importance.getSelectedItem().toString()))
            .title(title.getText().toString())
            .body(body.getText().toString())
            .location(location.getText().toString())
            // yyyy/MM/dd HH:mm:ss
            .timeInMilliseconds(getTimeInMillis(datePicker, timePicker))
            .build();
}

private long getTimeInMillis(DatePicker datePicker, TimePicker timePicker) {
    Calendar calendar = Calendar.getInstance();
    calendar.set(datePicker.getYear(), datePicker.getMonth(), datePicker.getDayOfMonth(), 
    timePicker.getCurrentHour(), timePicker.getCurrentMinute(), 0);
    return calendar.getTimeInMillis();
}

I don't think that extracting String objects from your textviews makes things any more readable, since your textviews are pretty clearly named.

edited Oct 27 '16 at 23:09

answered Oct 27 '16 at 22:32

Chantell Osejo

1,456
15
25

`SimpleDateFormat` would be a nice addition to the answer – OneCricketeer Oct 27 '16 at 22:38
I don't know what Timeconversion() is, or whether it does something different, or I'd be happy to update my answer. I guessed that the return value was a long based on the timeInMillis part. If OP wants to expound on the particular value, happy to optimize the answer. The key here is that since that particular chunk is a little hefty, putting that into a clearly-named method makes the code more readable. – Chantell Osejo Oct 27 '16 at 22:47
Obviously `date.getYear()+"/"+date.getMonth()` is getting a String. That was my point – OneCricketeer Oct 27 '16 at 22:48
Edit the answer if you'd like to put in SDF. I'll approve the edit. – Chantell Osejo Oct 27 '16 at 22:52
Timeconversion is a class that contains a method to convert a date string into milliseconds – mavi Oct 27 '16 at 22:56
And it would be a DatePicker and a TimePicker. I liked you idea so, I use a method like you. `private long getTimeInMillis(DatePicker date, TimePicker timePicker){ // yyyy/MM/dd HH:mm:ss return new Timeconversion() .timeConversion(date.getYear()+"/"+date.getMonth()+1+"/"+date.getDayOfMonth()+" " + timePicker.getCurrentHour()+":"+timePicker.getCurrentMinute() +":00"); }` – mavi Oct 27 '16 at 22:59
There are better ways to get milliseconds from integer values of year, month, day, hours and minutes without the space & time overhead of creating a number of String objects. But if you must use a TimeConversion class, why do you need to create an instance of it? It looks like the timeConversion() method should be static. – Klitos Kyriacou Oct 27 '16 at 23:03
Edited my answer...given new data, OP should be using Calendar to grab the time in milliseconds, like in this [answer](http://stackoverflow.com/a/13223266/4560689) – Chantell Osejo Oct 27 '16 at 23:10
That's fine, except that you are not clearing the milliseconds field when you use `Calendar.set()`, so you will have a random value between 0 and 999 added to the real number of milliseconds representing the time. I would instead do `Calendar calendar = new GregorianCalendar(year, month, day, hour, minute);` – Klitos Kyriacou Oct 27 '16 at 23:37

Which piece of code is better?

1 Answers1