1
\$\begingroup\$

I am using below code to rename and delete video.

Need review for the following:

  1. Code optimization

  2. Performance

  3. Overall code review

renameVideo.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                int index = filePath.lastIndexOf('.');
                String selectedVideoTitleForRename;
                if (index > 0) {
                    selectedVideoTitleForRename = filePath.substring(filePath.lastIndexOf("/") + 1, index);
                } else {
                    selectedVideoTitleForRename = filePath.substring(filePath.lastIndexOf("/") + 1);
                }
                renameFile(PreviewActivity.this, selectedVideoTitleForRename, filePath, tvInstruction);
            }
        });

        deleteVideo.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                deleteFile(PreviewActivity.this, filePath);
            }
        });
 private  void renameFile(final Context context, final String selectedVideoTitleForRename, final String selectedVideoRenamePath,
                                   final TextView tvInstruction) {
        AlertDialog.Builder alert = new AlertDialog.Builder(context);
        LayoutInflater li = LayoutInflater.from(context);
        View renameVideoView = li.inflate(R.layout.rename_video, null);
        final EditText input = (EditText) renameVideoView.findViewById(R.id.rename_edit_text);
        input.setText(selectedVideoTitleForRename);

        alert.setView(renameVideoView);
        alert.setNegativeButton("CANCEL", new DialogInterface.OnClickListener() {
            @Override
            public void onClick(DialogInterface dialog, int which) {
                dialog.cancel();
            }
        });
        alert.setPositiveButton("YES", new DialogInterface.OnClickListener() {
            @Override
            public void onClick(DialogInterface dialog, int which) {

                File fileToRename = new File(selectedVideoRenamePath);
                File fileNameNew = new File(selectedVideoRenamePath.replace(
                        selectedVideoTitleForRename, input.getText().toString()));
                if (fileNameNew.exists()) {
                    Toast.makeText(context,
                            context.getResources().getString(R.string.same_title_exists), Toast.LENGTH_LONG).show();
                } else {
                    boolean isRename = fileToRename.renameTo(fileNameNew);
                    if (isRename) {
                        String newFilePath = fileNameNew.toString();
                        tvInstruction.setText(context.getResources().getString(R.string.videoStoredPath, newFilePath));

                        try {
                            MediaScannerConnection.scanFile(context,
                                    new String[]{fileToRename.toString(), newFilePath},
                                    null,
                                    null);
                        } catch (Exception e) {
                            FirebaseDatabase.getInstance().getReference("exception").push().setValue(e.toString());
                            Crashlytics.logException(e);

                        }
                        filePath = newFilePath;
                        Toast.makeText(context,
                                context.getResources().getString(R.string.rename_success), Toast.LENGTH_LONG).show();
                    }
                }
            }
        });

        alert.show();

    }

    private void deleteFile(final Context context, final String selectedVideoDelete) {
        AlertDialog.Builder alertDialog = new AlertDialog.Builder(context);

        alertDialog.setTitle("Confirm Delete...");
        alertDialog.setMessage("Are you sure you want to Delete:\n\n" + selectedVideoDelete);
        alertDialog.setNegativeButton("NO", new DialogInterface.OnClickListener() {
            @Override
            public void onClick(DialogInterface dialog, int which) {
                dialog.cancel();
            }
        });
        alertDialog.setPositiveButton("YES", new DialogInterface.OnClickListener() {
            @Override
            public void onClick(DialogInterface dialog, int which) {
                File fileToDelete = new File(selectedVideoDelete);
                boolean deletedSuccessfully = fileToDelete.delete();
                if (deletedSuccessfully) {
                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {

                        MediaScannerConnection.scanFile(context,
                                new String[]{selectedVideoDelete}, null, null);

                    } else {
                        context.sendBroadcast(new Intent(
                                Intent.ACTION_MEDIA_MOUNTED,
                                Uri.parse("file://"
                                        + Environment.getExternalStorageDirectory())));
                    }
                    Toast.makeText(context,
                            context.getResources().getString(R.string.delete_success), Toast.LENGTH_LONG).show();
                    ((Activity) context).finish();

                }

            }
        });
        alertDialog.show();
    }
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Code Structuring

All Android tutorial code packs event listeners into anonymous inner classes, because the examples are usually short and it is convenient, from distribution point of view, to pack all code into the same file. It is however not the correct way to implement production code, as it creates large and messy compilation units that contain many many responsibilities, making them impossible targets for unit tests.

My personal rule of thumb is that if your OnClickListener or any other event listener contains anything more than one method call, it must be put it into a separate class. Otherwise a lambda is used.

I prefer strict adherence to single responsibility principle even in Android apps. I have used Guava EventBus in my hobby projects to send messages between different components and to avoid the "initialization spaghetti" where references between components are shared to connect them together. It works but you have to design the event mechanism throughly. There are several dependency injection libraries too, but I haven't had the time to look at those yet.

Code Optimization

We are dealing with an operation invoked by the user, which involves a confirmation dialog. What kind of problems have you noticed and what kind of preceivable optimization results are you looking for? To me this looks like premature optimization.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your answer..What u mean here by 'premature optimization'.Does this mean that code is already optimized? \$\endgroup\$ Commented Jun 23, 2020 at 7:22
  • 1
    \$\begingroup\$ It means that optimization should not be performed on code before it has been identified as being a bottle neck. The code in question is not run in a tight loop so any optimization would likely return minimal reward compared to the effort. \$\endgroup\$ Commented Jun 23, 2020 at 7:37
  • \$\begingroup\$ Sorry still not getting.. please explain in simpler language.what u mean by 'run in tight loop' and 'bottleneck' here? \$\endgroup\$ Commented Jun 23, 2020 at 7:40
  • 1
    \$\begingroup\$ It means, don't optimize the performance of a piece of code before knowing for sure that piece of code is contributing significantly to slow performance. It is time consuming for you to optimize code, and optimization often makes code less readable and maintainable. Don't spend ten minutes of your life making your users wait 1ms less for your dialog box. They won't be able to tell the difference. \$\endgroup\$
    – Tenfour04
    Commented Jun 23, 2020 at 20:20

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.