4
\$\begingroup\$

I am writing a clock in / out and payroll app for Android. I am no professional, but I was hoping I could get some feedback on whether my code is spaghetti or not. This is just one of many classes & activities, but I feel like this is one of the ones that is a little complex and I want to make sure I am not learning to write spaghetti code.

I particularly dislike the nested if block in the onCreate(), specifically the buttonClockOut.setOnClickListener(). Something feels off to me about this class in general. The code works as intended, I just feel like it could be improved in readability.

public class ClockOutActivity extends AppCompatActivity implements DialogAlreadyClockedInOrOut.DialogAlreadyClockedInOrOutListener {

    public static final int BUNDLE_CODE_CLOCK_OUT = 2;

    // TextClock
    private TextClock textClockClockOut, textClockClockOutDate;

    // EditText input fields
    private EditText editTextPasswordInput;

    // Buttons
    private Button buttonClockOut;

    // ViewModels
    private EmployeeViewModel employeeViewModel;
    private EmployeeWorkLogViewModel employeeWorkLogViewModel;

    // Local variables
    // ArrayLists
    private List<Employee> employeeList = new ArrayList<>();
    private List<EmployeeWorkLog> employeesAlreadyClockedIn = new ArrayList<>();

    // Strings
    private String dateOfLastClockInUI;
    private String employeeNameFromDB;
    private String employeeClockInTimeUI;

    // Booleans
    boolean isClockedInDB;
    private boolean forcedEmployeeBlunder = false;

    // ints
    private int fkEmpIdFromList;

    // Custom Objects
    EmployeeWorkLog employeeAlreadyClockedInEntry;


    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_clock_out);
        preliminarySetUp();

        setEmployeeList(); // employee list from all employee profiles in database

        // List of employees who are currently clocked in
        employeeWorkLogViewModel.getEmployeesAlreadyClockedIn().observe(this, new Observer<List<EmployeeWorkLog>>() {
            @Override
            public void onChanged(List<EmployeeWorkLog> employeeWorkLogs) {
                employeesAlreadyClockedIn = employeeWorkLogs;
            }
        });

        buttonClockOut.setOnClickListener(v -> {
            if (noEmptyETFields(editTextPasswordInput)) {
                int employeeIdUI = Integer.parseInt(editTextPasswordInput.getText().toString());
                String today = fetchDate();
                if (employeeExistsInDB(employeeIdUI)) {
                    if (employeeInAlreadyClockedInList(employeeIdUI) && employeeAlreadyClockedInEntry.getDate().equals(today)) {
                        // date of clock in matches date of intended clock out
                        normalClockOut();
                    } else {
                        showAlreadyClockedOutDialog();
                    }
                } else {
                    Toast.makeText(this, "ID not matched to Database. Try again.", Toast.LENGTH_SHORT).show();
                }
            } else {
                Toast.makeText(this, "You need to input an ID number first.", Toast.LENGTH_SHORT).show();
            }
        });

    }

    public void preliminarySetUp() {
        setTitle("Clock Out");
        defineViews();
        setTextClockFont();

    }

    public void defineViews() {
        this.editTextPasswordInput = findViewById(R.id.edit_text_employee_id_clock_out);
        this.buttonClockOut = findViewById(R.id.button_ok_clock_out);

        // ViewModels too I guess
        employeeViewModel = ViewModelProviders.of(this).get(EmployeeViewModel.class);
        employeeWorkLogViewModel = ViewModelProviders.of(this).get(EmployeeWorkLogViewModel.class);

    }

    public void setEmployeeList() {
        employeeViewModel.getAllEmployees().observe(this, new Observer<List<Employee>>() {
            @Override
            public void onChanged(List<Employee> employees) {
                employeeList = employees;
            }
        });
    }

    public void setTextClockFont() {
        textClockClockOut = findViewById(R.id.text_clock_clock_out);
        textClockClockOutDate = findViewById(R.id.text_clock_clock_out_date);
        //textClockDateMain.setFormat12Hour("EEEE MMM dd, yyyy");

        Typeface gothicFont = ResourcesCompat.getFont(this, R.font.gothic);
        textClockClockOut.setTypeface(gothicFont);
        textClockClockOutDate.setTypeface(gothicFont);
    }

    public boolean employeeExistsInDB(int employeeIdFromUI) {
        for (int i = 0; i < employeeList.size(); i++) {
            if (employeeList.get(i).getEmployeeIdNumber() == employeeIdFromUI) {
                employeeNameFromDB = employeeList.get(i).getEmployeeFirstName();
                fkEmpIdFromList = employeeList.get(i).getPEmpId();
                return true;
            }
        }
        return false;
    }

    public boolean employeeInAlreadyClockedInList(int employeeIdFromUI) {
        for (int i = 0; i < employeesAlreadyClockedIn.size(); i++) {
            if (employeesAlreadyClockedIn.get(i).getEmployeeId() == employeeIdFromUI) {
                // match, employee is already clocked in
                employeeAlreadyClockedInEntry = employeesAlreadyClockedIn.get(i);
                return true;
            }
        }
        return false; // match not found, employee not currently clocked in
    }

    public void normalClockOut() {
        String clockInTimeFromDB = employeeAlreadyClockedInEntry.getClockInTime();
        String clockOutTime = fetchTime();
        String totalShiftTime = calculateTotalShiftTime(clockInTimeFromDB, clockOutTime);

        // Sets these variables with the data from the last clock in entry of the employee
        int pId = employeeAlreadyClockedInEntry.getPId();
        int fkEmpId = employeeAlreadyClockedInEntry.getFkEmpId();
        String date = employeeAlreadyClockedInEntry.getDate();
        boolean isClockedIn = false;
        boolean employeeBlunder = false;
        if (forcedEmployeeBlunder) employeeBlunder = forcedEmployeeBlunder;
        int employeeId = employeeAlreadyClockedInEntry.getEmployeeId();

        EmployeeWorkLog employeeWorkLog = new EmployeeWorkLog(fkEmpId, date, clockInTimeFromDB,
                clockOutTime, totalShiftTime, isClockedIn, employeeBlunder, employeeId);
        employeeWorkLog.setPId(pId);

        employeeWorkLogViewModel.updateEmployeeWorkLog(employeeWorkLog);
        Toast.makeText(this, "Successfully clocked out at " + clockOutTime, Toast.LENGTH_LONG).show();
        finish();
    }

    public void forcedEmployeeClockIn() {
        int fkEmpIdFromLastObj = fkEmpIdFromList;
        int employeeIdNumberFromLastObj = Integer.parseInt(editTextPasswordInput.getText().toString().trim());
        boolean isClockedInCI = true;
        boolean employeeBlunderCI = true;

        EmployeeWorkLog forceCI = new EmployeeWorkLog(fkEmpIdFromLastObj, fetchDate(),
                employeeClockInTimeUI, null, null, isClockedInCI,
                employeeBlunderCI, employeeIdNumberFromLastObj);

        employeeWorkLogViewModel.insertEmployeeWorkLog(forceCI);

        employeeWorkLogViewModel.getEmployeesAlreadyClockedIn().observe(this, new Observer<List<EmployeeWorkLog>>() {
            @Override
            public void onChanged(List<EmployeeWorkLog> employeeWorkLogs) {
                employeesAlreadyClockedIn = employeeWorkLogs;
                resetList(employeeIdNumberFromLastObj);
            }
        });
        // old re-attempt code lived here
    }

    public void resetList(int employeeIdNumberFromLastObj) {
        for (int i = 0; i < employeesAlreadyClockedIn.size(); i++) {
            if (employeesAlreadyClockedIn.get(i).getEmployeeId() == employeeIdNumberFromLastObj) {
                employeeAlreadyClockedInEntry = employeesAlreadyClockedIn.get(i);
                forcedEmployeeBlunder = true;
                // Re-attempt normal clock out
                normalClockOut();
            }
        }
    }

    public String fetchDate() {
        DateFormat dateFormat = new SimpleDateFormat("MM-dd-yyyy");
        String date = dateFormat.format(Calendar.getInstance().getTime());
        return date;
    }


    public String fetchTime() {
        DateFormat dateFormat = new SimpleDateFormat("HH:mm:ss");
        String time = dateFormat.format(Calendar.getInstance().getTime());
        return time;
    }

    public boolean noEmptyETFields(EditText editText) {
        return !editText.getText().toString().trim().isEmpty();
    }

    public String calculateTotalShiftTime(String clockInTime, String clockOutTime) {
        String totalShiftTime;

        String[] clockInTimeArray = clockInTime.split(":");
        String[] clockOutTimeArray = clockOutTime.split(":");
        int timeTotalHours = 0;
        int timeTotalMinutes = 0;

        for (int i = 0; i < clockOutTimeArray.length; i++) {
            int timeCO = Integer.parseInt(clockOutTimeArray[i]);
            int timeCI = Integer.parseInt(clockInTimeArray[i]);

            switch (i) {
                case 0: // Hours slot
                    timeTotalHours = timeCO - timeCI;
                    break;
                case 1: // minutes slot
                    if (timeCO > timeCI) {
                        timeTotalMinutes = timeCO - timeCI;
                    } else {
                        timeTotalMinutes = timeCO - timeCI;
                        timeTotalMinutes += 60;
                        timeTotalHours--;
                    }
                    break;
            }
        }
        totalShiftTime = timeTotalHours + ":" + timeTotalMinutes;

        return totalShiftTime;
    }

    public void showAlreadyClockedOutDialog() {
        DialogAlreadyClockedInOrOut dialog = new DialogAlreadyClockedInOrOut();

        Bundle bundle = new Bundle();
        bundle.putInt("bundleCode", BUNDLE_CODE_CLOCK_OUT);
        bundle.putString("bundleName", employeeNameFromDB);
        dialog.setArguments(bundle);
        dialog.show(getSupportFragmentManager(), "prompt - DialogAlreadyClockedInOrOut - OUT");
    }

    // Will receive the input from the dialog where the user typed in the
    // time they should've clocked out
    @Override
    public void applyUserTimeData(String uTime) {
        // adding :00 to the end of the string allows the comparison from the calculateTotalShiftTime
        // to proceed without out of bounds errors
        employeeClockInTimeUI = uTime + ":00";
        forcedEmployeeClockIn();
    }
}
\$\endgroup\$
0
\$\begingroup\$

Let me know if you ever see an Android activity class that is not spaghetti. I'm eager to learn if it is possible...

When UI code and business logic is combined into one class, the code becomes harder to read as the reader constantly needs to remember whether a field is related to UI or business logic. The code related to maintaining the payroll-related data should be placed into a separate package (putting them into a separate Maven project wouldn't be too extreme) and the Android-part should be reserved for as pure UI code as poosible.

Lambdas should be short and simple. Multi-line lambdas shoul be extracted into standalone classes. For some reason Android Java code heavily promotes anonymous classes. In my opinion they make the code hard to read and if the programmer is not diligent, results in single responsibility violations.

The activity class itself acts both as an activity and a listener. That's a clear single responsibility violation. Every single Android tutorial does it because it makes the sample code shorter. In the real world it's usually just bad practise.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for reviewing my code @TorbenPutkonen! I will do my best to refactor my lambdas on all my classes. As far as separating the business logic from the UI, I am currently using a ViewModel class for my Room library database queries only. I was wondering if other than using Maven, would it be sensible to move my business logic for any clock in/out functions into their own ViewModel classes instead? \$\endgroup\$ – Elizabeth Sinclair May 8 at 19:48
  • \$\begingroup\$ It would be sensible. \$\endgroup\$ – TorbenPutkonen May 9 at 5:10

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.