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.

Can anyone give me feedback please? I used what I have learnt so far, mainly methods, loops and arrays. I would like you view based on these topics, however, comments on how to improve are welcome.

/*ask the user for 10 numbers
store them in an array
print them "your numbers are:"
print them "Average"
print them "Max"
print them "Min"*/

import javax.swing.*;

public class homeworkArrayWFunctionstest5
{
    public static void main(String[] args) throws Exception
    {
        String listOfNums = "";
        int sum = 0;
        int population = 0;
        int average = 0;
        int maxValue = 0;
        int minValue = 0;
        String [] numsStringInput = new String[10];
        population = numsStringInput.length;
        int [] arrayNumInt = new int[population];

        numsStringInput     = UserInputs(numsStringInput,population);
        arrayNumInt         = StringToInt(numsStringInput,population);
        listOfNums          = OneLineList(numsStringInput, population);
        sum                 = TotalCalc(arrayNumInt,population);
        average             = AvarageCalc(sum, population);
        maxValue            = MaxValueCalc(arrayNumInt,population);
        minValue            = MinValueCalc(arrayNumInt,population);

        Output(listOfNums,average,minValue,maxValue);
    }

    public static String [] UserInputs(String []numsStringInput,int population)
    {
        for (int i = 0; i<population; i++)
        {
            numsStringInput [i] = JOptionPane.showInputDialog(null,"Please Input number " + (i+1) + " of " + population + " numbers","Input Table",JOptionPane.QUESTION_MESSAGE);
        }

        return numsStringInput;
    }

    public static int [] StringToInt(String[] numsStringInput,int population)
    {
        int [] arrayNumInt = new int[population];

        for (int i = 0; i<population; i++)
        {
            arrayNumInt [i] = Integer.parseInt(numsStringInput [i]);
        }
        return arrayNumInt;
    }

    public static String OneLineList(String[] numsStringInput,int population)
    {
        String listOfNums = "";
        for (int i = 0; i<population; i++)
            {
                listOfNums = listOfNums + numsStringInput [i] + " ";
            }

        return listOfNums;
    }

    public static int TotalCalc(int []arrayNumInt,int population)
    {
        int total = 0;
        for(int i=0;i < population;i++)
        {
            total = total + arrayNumInt[i];
        }
        return total;
    }

    public static int AvarageCalc(int sum,int population)
    {
        int avarage = sum / population;
        return avarage;
    }

    public static int MaxValueCalc(int []arrayNumInt,int population)
    {
        int maxValue = arrayNumInt[0];
        for(int i=1;i < population;i++)
        {
            if(arrayNumInt[i] > maxValue)
            {
              maxValue = arrayNumInt[i];
            }
        }
        return maxValue;
    }

    public static int MinValueCalc(int []arrayNumInt,int population)
    {
        int minValue = arrayNumInt[0];
        for(int i=1;i < population;i++)
        {
            if(arrayNumInt[i] < minValue)
            {
              minValue = arrayNumInt[i];
            }
        }
        return minValue;
    }

    public static void Output(String listOfNums, int average, int minValue,int maxValue )
    {
        String text = "";

        String messageText[] = new String [] {"The number you have entered are:","Average is: ","Minimum number is: ","Maximum number is: "};
        int [] textNum = new int[] {0,average,minValue,maxValue};

        text = messageText[0]+ "\n" + listOfNums;

        for (int i = 1; i<messageText.length; i++)
        {
            text = text + "\n" + messageText[i] + textNum[i];
        }
        JOptionPane.showMessageDialog(null,text,"Output Table" ,JOptionPane.INFORMATION_MESSAGE);
    }
}
share|improve this question

2 Answers 2

  • The Java standard dictates the use of the "Egyptian braces" style:

    if (something) {
        // something...
    }
    
  • In AvarageCalc():

    int avarage = sum / population;
    return avarage;
    

    you can just return the calculation:

    return sum / population;
    

    Also, it should be spelled average. ;-)

  • In Output(), you don't need to declare text and then assign it later. Instead, initialize it where it's first used. Always try to keep variables as close in scope as possible, which can help keep your code cleaner and easier to maintain.

    String text = messageText[0]+ "\n" + listOfNums;
    
share|improve this answer
    
Thanks for your feedback! the only thing I could not figure out is •The Java standard dictates the use of the "Egyptian braces" style:.......I think I have them in place! –  user3693592 Jun 17 at 21:32
    
@user3693592: What you have is different. Egyptian style places the opening brace on the same line as the statement, not on its own line. –  Jamal Jun 17 at 21:35
    
Got it ...Thanks! –  user3693592 Jun 17 at 21:40
1  
@user3693592: Now that you have 15 rep, you can now start voting on posts you've found useful. You may eventually get more thorough answers from others. –  Jamal Jun 17 at 22:30

Naming conventions

Classes start with an uppercase letter, methods start with lowercase letters.

HomeworkArrayWFunctionstest5 would be slightly better, but HomeworkArrayWithFunctionsTest5 would be even better, although describing what the class is used for is often best. That's hard to apply on homework though.

Following the conventions, totalCalc is a better name on the method, or even better: sum.

Right aligning code does not feel right.

I do not often see code like this in Java.

    numsStringInput     = UserInputs(numsStringInput,population);
    arrayNumInt         = StringToInt(numsStringInput,population);
    listOfNums          = OneLineList(numsStringInput, population);
    ...

I would actually, not care much about the right aligning there. This is just my opinion though, I am not aware of any conventions regarding this. If you like it then feel free to continue doing so (for now at least).

Inconsistent Spacing.

Compare

for (int i = 0; i<population; i++)

with

for(int i=0;i < population;i++)

(Yes, both of these are from your code)

The cleanest way of spacing this is:

for (int i = 0; i < population; i++)

You're also inconsistent with arrays: int []arrayNumInt or String[] numsStringInput? Prefer the second one. Using spacing like Type[] variableName is better.

And your method calls:

... StringToInt(numsStringInput,population);
... OneLineList(numsStringInput, population);

Always put a space after a comma! Just like when you write normal text, right? (Also rename these methods as suggested above, and after applying one suggestion below you'll not even need a comma here at all).

Unnecessary throws declaration

Your main method is declared to throws Exception there is no need to do this as all exceptions that can be thrown are RuntimeExceptions, which are not checked exceptions. Unchecked exceptions will be thrown anyway if needed. Only declare throwing exceptions when you need to. And when you do need to (the time will come when you need to), don't declare it as the general type Exception but instead one of the subclasses (IOException or whatever).

Other comments

  • arrayNumInt does not need to be created the first time.

  • The population parameter to your methods can be removed and replaced by arrayNumInt.length. As it currently stands it's just a dangerous parameter, consider what would happen if it's value did not match the array length. (I dare you to test it! And understand why whatever happens then happens!)

  • AvarageCalc is so simple it doesn't need to be a method. Your taking one line of code, putting it into a method and replacing it by another line of code. For more complex lines of code, I would understand this. But for sum / population, there's just no need to.

  • @Jamal said "In Output(), you don't need to declare text and then assign it later.", this also applies to a lot of variables in your main method. You can just use:

    int sum = totalCalc(arrayNumInt);
    

(After renaming the method and removing the unnecessary population parameter)

share|improve this answer
1  
Right alignment can look good when it is written. However, it is hard to maintain. Every time a new item is added, there is a chance that all of the lines might have to be altered. Because of this, it is not advised. Additionally, I don't know of an automated code formatter that would retain the extra spaces before the equals sign. –  unholysampler Jun 18 at 0:48
    
Thanks for your review and comments, especially to Simon. Thats the type of support I was looking for. Much appriciated. –  user3693592 Jun 19 at 3:59

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.