2010-05-06 30 views
2

這是比較簡單的程序。但我想獲得一些關於如何改進此計劃(如果有)的反饋,例如,不必要的陳述?對這個指針程序給予反饋

#include<iostream> 
#include<fstream> 
using namespace std; 

double Average(double*,int); 

int main() 
{ 

    ifstream inFile("data2.txt"); 

    const int SIZE = 4; 
    double *array = new double(SIZE); 
    double *temp; 
    temp = array; 

    for (int i = 0; i < SIZE; i++) 
    { 
     inFile >> *array++; 
    } 
    cout << "Average is: " << Average(temp, SIZE) << endl; 
} 

double Average(double *pointer, int x) 
{ 
    double sum = 0; 

    for (int i = 0; i < x; i++) 
    { 
     sum += *pointer++; 
    } 
    return (sum/x); 
} 

該代碼是有效的,程序工作正常。但我只是想聽聽你們的想法,因爲你們大多數人的經驗比我多(我只是一個大一新生......大聲笑)

謝謝。

+0

我會讓這個程序更長。在命令行上傳入文件名。檢查文件是否存在。如果不是,則以良好的方式錯誤返回-1或1.另外,更喜歡普通的舊數組指向指針。額外的指針是一個很好的學習練習,但更糟的是產品代碼(除非有特定的限制)。 – 2010-05-06 03:15:54

+0

是的。最初這個程序是用數組編寫的。但我寫了這個,因爲我們剛剛完成了指針。所以我花了幾分鐘修改舊程序的前半部分。好主意,Hamish! – CppLearner 2010-05-06 03:23:07

回答

4

修復內存泄漏。 即刪除溫度; 此外,檢查文件被打開/ created..etc

理想情況下,你應該用你的臨時變量,而不是使用*陣列本身

+0

另外,處理空文件,部分填充的文件。如果該文件有超過4個條目會怎麼樣?在'double Average(double *,int);'中放置'double *,'和'int'之間的空格。 – 2010-05-06 03:17:13

+0

我沒有得到第三個陳述「遍歷數組...」,你可能會更詳細地描述一下它嗎?謝謝! – CppLearner 2010-05-06 03:19:19

+0

@ johnWong - 「遍歷數組......」就是你現在正在做的事情。在for循環中一次訪問一個元素,直到最後一個 – SysAdmin 2010-05-06 03:32:48

1

如果x爲0,則平均將產生操縱/數組遍歷除零誤差。

+0

沒錯,可以添加額外的警告。偉大的想法! – CppLearner 2010-05-06 03:18:34

+0

x是負數時怎麼樣?你想處理異常還是使用C風格的errno,其他?到目前爲止,沒有真正的C++。我敢打賭,這個程序會用C編譯器編譯。 – 2010-05-06 03:23:20

+0

@Hamish:我敢打賭,它沒有使用'new'和iostreams。 – 2010-05-06 04:01:50

2

既然我們在談論C++,我會建議使用STL容器和算法。我還發現,在大多數情況下,最好使用引用或智能指針(例如boost :: shared_ptr)而不是原始指針。在這種情況下,根本不需要指針。

這裏是你如何可以寫你的程序:

#include <fstream> 
#include <vector> 
#include <iostream> 
#include <numeric> 
#include <iterator> 

using namespace std; 

int main() 
{ 
    ifstream f("doubles.txt"); 
    istream_iterator<double> start(f), end; 
    vector<double> v(start, end); 

    if (v.empty()) 
    { 
     cout << "no data" << endl; 
     return 0; 
    } 

    double res = accumulate(v.begin(), v.end(), 0.0); 
    cout << "Average: " << res/v.size() << endl; 
    return 0; 
} 
+0

wooo這是非常先進的。我們確實觸及了STL和向量,但不是太多。感謝您花費時間,這是一個很好的參考。 – CppLearner 2010-05-06 03:43:25

4

您沒有正確初始化您的數組。本聲明:

double *array = new double(SIZE); 

分配一個double並將其初始化爲SIZE的值。你應該做使用數組分配是:

double *array = new double[SIZE]; 

的另一個普遍問題是你從來也不需要動態分配的內存分配給原始指針。如果你想使用基本類型,而不是更高層次的對象,如std::vector,你應該總是使用智能指針:

boost::scoped_array<double> array(new double[SIZE]); 

現在陣列將自動獲得不管你如何從新留下您的範圍(即釋放增加回報或來自例外)。

+0

嗨,謝謝。感謝你指出了這兩個區別。新的雙[SIZE]是一個數組分配,對。我確實將內存分配視爲線性排列,我認爲這是一個數組。 ()我仍然不太明白這個問題。我確實看到你關於數組分配的觀點。 – CppLearner 2010-05-06 03:54:45

+0

@JohnWong - 'new double(SIZE)'分配** 1 **雙對象。就其本身而言,沒有任何問題;問題在於代碼將其視爲導致「緩衝區溢出」的數組。 – 2010-05-06 04:05:10

0

這裏有一些 「代碼審查」 的評論:

在main():

  1. 改變大小,以 「size_噸」,而不是int
  2. 爲什麼SIZE是大寫? (可能是筆者的習慣是有常量爲大寫,在這種情況下,它是好的。)
  3. 聯合臨時申報和分配到一個語句爲:double * temp = array;
  4. 如果​​不可用,或者無法打開閱讀?
  5. 如果​​的項目數小於SIZE,該怎麼辦?
  6. 將循環變量i更改爲size_t
  7. 在宣佈​​之前刪除空白行。
  8. main()返回一些數字(例如0)。
  9. 更正數組的分配。

在平均():

  1. 平均變化到size_t第二個參數。
  2. 指針的斷言和/或警衛非空
  3. 斷言和/或防止被零除。

確認:有些點是從其他答案中收集的。我儘可能地做出完整的清單。

+0

感謝您的意見。我真的很喜歡這種談話。這個討論反映了我仍然需要學習多少:)所以對於size_t,我需要指定多少? – CppLearner 2010-05-06 04:01:25