Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've always found coding UIs in Java very inefficient, and hopefully this is because of my misunderstanding. Here's a great example.

I have a UI class which is returning the String "Hours", which is effectively going to be a CSV file containing opening and closing hours of a business, Monday through Sunday. Each of these values come from individual combo boxes. So to build this String, I was curious - is there any possible way to improve it through, say, loops or some kind of recursive function?

Otherwise it's incredibly difficult to read. Any suggestions how to improve this disgusting code?

String hours = cbMondayStart.getItemAt(cbMondayStart.getSelectedIndex()) + "," +
    cbMondayFinish.getItemAt(cbMondayFinish.getSelectedIndex()) + "," +
    cbTuesdayStart.getItemAt(cbTuesdayStart.getSelectedIndex()) + "," +
    cbTuesdayStart.getItemAt(cbTuesdayStart.getSelectedIndex()) + "," +
    cbTuesdayFinish.getItemAt(cbTuesdayFinish.getSelectedIndex()) + "," +
    cbWednesdayStart.getItemAt(cbWednesdayStart.getSelectedIndex()) + "," +
    cbWednesdayFinish.getItemAt(cbWednesdayFinish.getSelectedIndex()) + "," +
    cbThursdayStart.getItemAt(cbThursdayStart.getSelectedIndex()) + "," +
    cbThursdayFinish.getItemAt(cbThursdayFinish.getSelectedIndex()) + "," +
    cbFridayStart.getItemAt(cbFridayStart.getSelectedIndex()) + "," +
    cbFridayFinish.getItemAt(cbFridayFinish.getSelectedIndex()) + "," +
    cbSaturdayStart.getItemAt(cbSaturdayStart.getSelectedIndex()) + "," +
    cbSaturdayFinish.getItemAt(cbSaturdayFinish.getSelectedIndex()) + "," +
    cbSundayStart.getItemAt(cbSundayStart.getSelectedIndex()) + "," +
    cbSundayFinish.getItemAt(cbSundayFinish.getSelectedIndex());
share|improve this question

migrated from stackoverflow.com Aug 15 at 15:56

This question came from our site for professional and enthusiast programmers.

4  
How about putting the references of the elements in a list when you create them, and using a loop? – RealSkeptic Aug 13 at 16:20
4  
String hours = comboBoxes.stream().map(c -> String.valueOf(c.getSelectedIndex()).collect(Collectors.joining(","); – JB Nizet Aug 13 at 16:24
    
Rule 1 of programming: don't blame the compiler. Obvious parallel: don't blame the programming language. – DavidS Aug 13 at 16:50

1 Answer 1

up vote 3 down vote accepted

Look for repeated code and pull it out:

Stream.of(cbMondayStart, cbMondayFinish, cbTuesdayStart, cbTuesdayFinish, ...)
    .map(e -> e.getItemAt(e.getSelectedIndex()))
    .map(String::valueOf)
    .collect(Collectors.joining(","));
share|improve this answer
    
Works a charm, had this code in the end for anyone else looking for a finale: String hours = Stream .of(cbMondayStart, cbMondayFinish, cbTuesdayStart, cbTuesdayFinish, cbWednesdayStart, cbWednesdayFinish, cbThursdayStart, cbThursdayFinish, cbFridayStart, cbFridayFinish, cbSaturdayStart, cbSaturdayFinish, cbSundayStart, cbSundayFinish) .map(element -> element.getItemAt(element .getSelectedIndex())).map(String::valueOf) .collect(Collectors.joining(",")); – Ken Reid Aug 13 at 16:48

Your Answer

 
discard

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