Selection Sort in C++ I wrote a little C++ sorting routine for my Algorithm studies, but I feel that a lot can be skipped out.

I am aware that modern sort takes Iterators and comparator functions, let's disregard that for now. I just want to clean up this code. Specifically, I don't like the if and value variables.

I have looked into other solutions, but they didn't clear up much for me.

void SelectionSorter::sort(std::vector<int> &v){

    int currentIndex = 0;

    while (currentIndex < v.size()) {

        int indexOfSmallestItem = -1;
        int value = v[currentIndex];
        bool shouldSwap = false;

        for (int i = currentIndex; i < v.size(); i++) {
            if (v[i] < value) {
                value = v[i];
                indexOfSmallestItem = i;
                shouldSwap = true;
            }
        }

        if (shouldSwap) {
            int temp = v[currentIndex];
            v[currentIndex] = v[indexOfSmallestItem];
            v[indexOfSmallestItem] = temp;
        }

        currentIndex++;
    }
}

Some points:

The shouldSwap variable is not needed. Instead, set indexOfSmallestItem to currentIndex at the beginning of the loop (this is necessary for the next step to work) and check is the indexOfSmallestItem is currentIndex:

if (indexOfSmallestItem != currentIndex) {
    int temp = v[currentIndex];
    v[currentIndex] = v[indexOfSmallestItem];
    v[indexOfSmallestItem] = temp;
}

value is also not needed, as you can access the vector directly:

    if (v[i] < v[indexOfSmallestItem]) {
        value = v[i];
        indexOfSmallestItem = i;
    }

The outside while loop can be a for loop, because:

int currentIndex = 0; // initialization

while (currentIndex < v.size()) { // condition

    // ...

    currentIndex++; // increment
}

So it would be:

for (int currentIndex = 0; currentIndex < v.size(); currentIndex++) {
    // ...
}

You actually can start on currentIndex + 1 because the first one is the current value.
The naming of the vector is not good, as one-letter names are hard to read. Change it to vect or something similar.

Now all you need is the indexOfSmallestItem and the vector:

void SelectionSorter::sort(std::vector<int> &vect) {

    for (int currentIndex = 0; currentIndex < vect.size(); currentIndex++) {

        int indexOfSmallestItem = currentIndex;

        for (int i = currentIndex + 1; i < vect.size(); i++) {
            if (vect[i] < vect[indexOfSmallestItem]) {
                indexOfSmallestItem = i;
            }
        }

        if (indexOfSmallestItem != currentIndex) {
            int temp = vect[currentIndex];
            vect[currentIndex] = vect[indexOfSmallestItem];
            vect[indexOfSmallestItem] = temp;
        }

    }

}

Optionally, you can remove the if (indexOfSmallestItem != currentIndex) completely, as it is not much of an improvement.

Also, you can extract the swapping part into a method. In fact, you should. This makes code (even this size) easier to maintain.

void SelectionSorter::sort(std::vector<int> &vect) {
    for (int currentIndex = 0; currentIndex < vect.size(); currentIndex++) {
        int indexOfSmallestItem = currentIndex;
        for (int i = currentIndex + 1; i < vect.size(); i++) {
            if (vect[i] < vect[indexOfSmallestItem]) {
                indexOfSmallestItem = i;
            }
        }
        swap(vect, currentIndex, indexOfSmallestItem);
    }
}

void swap(std::vector<int> &vect, int i, int j) {
    int temp = vect[i];
    vect[i] = vect[j];
    vect[j] = temp;
}


Using enum descriptions to string/text binding In my WP8 application I need to show a couple of radio buttons to let the user select a value out of an enum. I don't want to hardcode either the value or the text (description) showed to the user. I would like my Model/ViewModel drive them for me.

As of now, this is what I have:

//enum defined in model
 public enum Units
    {
        [Description ("Meters/km")]
        Metric,
        [Description ("Feet/Miles")]
        Imperial
    }

Converters defined to handle string and Boolean conversions

//Converts ENum to boolean and back
// Convert: uses parameter passed in, returns true if current value of the Enum matches parameter
//ConvertBack: if value is true, sets the value of the ENum to parameter passed in
//
[ValueConversion (typeof (Enum), typeof (Boolean))]
public class EnumToBooleanConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        string parameterString = parameter as string;
        if ( parameterString == null )
            return DependencyProperty.UnsetValue;

        if ( Enum.IsDefined (value.GetType (), value) == false )
            return DependencyProperty.UnsetValue;

        object parameterValue = Enum.Parse (value.GetType (), parameterString);

        return parameterValue.Equals (value);

    }

    public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        return value.Equals (true) ? Enum.Parse(targetType, parameter as String) : DependencyProperty.UnsetValue;
    }
}

//Converts ENum to strings, uses Description for the parameter passed in, or parameter as string
[ValueConversion (typeof (Enum), typeof (String))]
public class EnumToStringConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        string parameterString = parameter as string;
        if ( parameterString == null )
            return DependencyProperty.UnsetValue;

        if ( Enum.IsDefined (value.GetType (), value) == false )
            return DependencyProperty.UnsetValue;

        var desc = (value.GetType ().GetField (parameterString).GetCustomAttributes (typeof (DescriptionAttribute), false).FirstOrDefault() as DescriptionAttribute);
        if ( desc != null )
            return desc.Description;
        else
            return parameter.ToString ();
    }

    public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        throw new NotImplementedException ();
    }
}

In the XAML:

   <Grid  Margin="0,10,0,0">
    <Grid.RowDefinitions>
        <RowDefinition />
        <RowDefinition />
    </Grid.RowDefinitions>
    <Grid.ColumnDefinitions>
        <ColumnDefinition Width="Auto" />
        <ColumnDefinition Width="Auto" />
    </Grid.ColumnDefinitions>
    <TextBlock Text="How do you measure distance?"
               Margin="6,0,0,0"
               Grid.ColumnSpan="2"
               Grid.Row="0" />
    <RadioButton Grid.Row="1"
                 Grid.Column="0"
                 Content="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToStringConverter}}"
                 IsChecked="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />
    <RadioButton Grid.Row="1"
                 Grid.Column="1"
                 Content="{Binding PreferredUnit, ConverterParameter=Imperial, Converter={StaticResource EnumToStringConverter}}"
                 IsChecked="{Binding PreferredUnit, ConverterParameter=Imperial, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />
</Grid>

Does anyone here think I am on the right track with my approach? Any suggestions on improving the code (in terms of logic, clarity, performance)?

This looks perfectly reasonable.  In fact, the only thing I would really change is this:

string parameterString = parameter as string;
if ( parameterString == null )
    return DependencyProperty.UnsetValue;

if ( Enum.IsDefined (value.GetType (), value) == false )
    return DependencyProperty.UnsetValue;

You have this in both Convert methods, and it would be just as clear like this:

string parameterString = parameter as string;
if ( parameterString == null || !Enum.IsDefined (value.GetType (), value) )
    return DependencyProperty.UnsetValue;

Additionally, you could concatenate this:

object parameterValue = Enum.Parse (value.GetType (), parameterString);
return parameterValue.Equals (value);

Like this:

return Enum.Parse (value.GetType(), parameterString).Equals (value);

For your XAML, you do not need to put each property on its own individual line.  I typically only break them up when they do not fit on my screen all at once, more like this

<TextBlock Text="How do you measure distance?" Margin="6,0,0,0" Grid.ColumnSpan="2" Grid.Row="0" />
<RadioButton Grid.Row="1" Grid.Column="0"
             Content="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToStringConverter}}"
             IsChecked="{Binding PreferredUnit, ConverterParameter=Metric, Converter={StaticResource EnumToBooleanConverter}, Mode=TwoWay}" />

Finally, for your UI, you most likely want to label your radio buttons as what they are, and perhaps change your UI a little to accommodate the labels and still look good.  I would recommend a ComboBox if you have trouble adjusting the UI to look good.  You could then bind the SelectedItem or SelectedIndex to a value in your VM and use a converter to change the value as needed.

<ComboBox Text="Label">
    <ComboBoxItem>Metric (meters)</ComboBoxItem>
    <ComboBoxItem>Imperial (feet)</ComboBoxItem>
</ComboBox>


Project Euler 78: Counting Partitions Project Euler problem 78:

  Let \$p(n)\$ represent the number of different ways in which \$n\$ coins can be separated into piles. For example, five coins can be separated into piles in exactly seven different ways, so \$p(5)=7\$.

  Find the least value of \$n\$ for which \$p(n)\$ is divisible by one million.

I used Euler's recurrence relation for the partition of \$n\$, together with the fact that modular addition is distributive: $$(a + b) \bmod n = ((a \bmod n) + (b \bmod n)) \bmod n.$$

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

int main()
{
  // generalized pentagonal numbers
  vector<int> g = vector<int>({1, 2}); // g_1 = 1, g_-1 = 2
  // integer partitions
  vector<int> p = vector<int>({1}); // p_0 = 1
  for (int n = 1; n ; n++) {
    if (g.back() <= n) {
      int k = g.size()/2 + 1;
      g.push_back(k*(3*k - 1)/2);
      k *= -1;
      g.push_back(k*(3*k - 1)/2);
    }
    int p_n = 0;
    for (vector<int>::iterator g_k = g.begin(); *g_k <= n; ++g_k) {
      if ((g_k - g.begin())/2 % 2 == 0) {
        p_n += p[n - *g_k];
      } else {
        p_n -= p[n - *g_k];
      }
    }
    p.push_back(p_n % 1000000);
    if (p.back() < 0) p.back() += 1000000;
    if (p.back() == 0) {
      cout << n << endl;
      break;
    }
  }
  return 0;
}

My i7 handles this in ~25ms, but my circa 2008 32-bit centrino needs 600ms. Is there any way that I can squeeze more performance out of the above code? Also, with the exception of removing using namespace std;, is there anything I can do to make the code cleaner/more readable/more presentable?

I'll just comment on the cleanliness/readability aspect:

In spite of the given comments, you should give g and p better names.  It'll also allow you to remove those comments, which is good because they shouldn't need to be used as such.
This kind of initialization isn't really necessary:

vector<int> g = vector<int>({1, 2});

It could just be this:

vector<int> g = { 1, 2 };

If you're using C++11, you can also omit the =.
This loop statement may be a little hard to read:

for (int n = 1; n ; n++)

It's understood that having n alone means the loop will continue as long as it's nonzero, but you can still make that clear here anyway.  It's otherwise hard to read this statement at a glance.
This calculation is unclear as written:

g.push_back(k*(3*k - 1)/2);

You also reuse it, so consider making it a constant in terms of k.
Try to add more blank lines between code sections, such as the loops.  Having everything together like this can make it harder for the brain to parse out different sections easily.
The number 1000000 here is a "magic number" (ambiguous hard-coded number).  As such, it should be assigned to a constant with a relevant name.  This will also allow you to change this number in just one place.


Extract and divide algorithm implementation in Elixir I just started learning Elixir and stumbled upon this challenge over on Programming Puzzles & Code Golf. It is a well-suited task for beginners, so I chose to give it a go (to be clear, give it a go means solve it normally, not golfing it). To keep this question self-contained, here is the task – citing the linked post:

  For a given positive integer \$n\$:

  Repeat the following until \$n < 10\$ (until \$n\$ contains one digit).
  Extract the last digit.
  If the extracted digit is even (including 0) multiply the rest of the integer by \$2\$ and add \$1\$ ( \$2n+1\$ ). Then go back to step 1 else move to step 4.
  Divide the rest of the integer with the extracted digit (integer / digit) and add the remainder (integer % digit), that is your new \$n\$.

For example, \$61407\$ gives \$5\$ when ran through this mechanism. I've come up with the following code:

defmodule ExtractAndDivide do
  def extract_and_divide(x) do
    if x < 10 do x
    else
      head = div x, 10
      tail = rem x, 10
      case rem tail, 2 do
        0 -> head * 2 + 1 |> extract_and_divide
        1 -> div(head, tail) + rem(head, tail) |> extract_and_divide
      end
    end
  end
end

I'm seeking general advice, but mainly focusing on the following:

Naming and Syntax better practices (usage of parenthesises, variable names etc.)
Usage of |> (pipe) in this context. Would you ever see it used the way I did it in production code? Should I switch to "normal" notation instead?
Less verbose or more elegant way to avoid the seemingly unaesthetic if x < 10 do x ... else ... end structure, perhaps using case would be better here?
Is recursion the way to go? Should I stick to it or are there better, equivalent methods?

(untested)

defmodule ExtractAndDivide do
  def extract_and_divide(x) when x < 10, do: x
  def extract_and_divide(x) do
    head = div x, 10
    tail = rem x, 10
    up_or_down = rem tail, 2
    go_up_or_down(head, tail, up_or_down)
  end
  defp go_up_or_down(head, tail, up_or_down) when up_or_down == 0 do
    extract_and_divide(head * 2 + 1)
  end
  defp go_up_or_down(head, tail, _up_or_down_is_one) do
    extract_and_divide(div(head, tail) + rem(head, tail))
  end
end

The idea with having multiple conditional function clauses is twofold: it reduces nesting (and thus should increase readability), and - quite powerful - it allows to to easily test individual conditional parts. Whether to make the recursive call to extract_and_divide by piping the calculation through the function invocation or directly (like I did purely to show the alternative) is largely a matter of taste.

(note: skipped/circumvented potential nitpicking around naming and whether up_or_down shouldn't be a boolean; I just wanted to illustrate the pattern of doing logic by using multiple function heads)


Conditionals validation for Tic Tac Toe I have been working with Java for a little more than a year. I recently have built a Tic Tac Toe game as an assignment for my Java class. After my instructor graded it, he wrote a comment around my validation method logic. Even though I got 100%, he said that the logic in my validation method is too cumbersome. He stated that I should look into a for orwhile` statement in order to clean out some code in my validation method.

Is there really a way to put all my conditional if statements in to a for or while loop? And if so, I would like to know what logic goes behind that. This program had a set of five arrays but in this validation method I worked only with the JButton array.

JButton [] button = new JButton [9];
public void validate()
    {
        if(button[0].getText().equals(button[1].getText()) && button[1].getText().equals(button[2].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[0].getText());
            gameOver();
            return;
        }
        else if(button[3].getText().equals(button[4].getText()) && button[4].getText().equals(button[5].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[3].getText());
            gameOver();
            return;
        }
        else if(button[6].getText().equals(button[7].getText()) && button[7].getText().equals(button[8].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[6].getText());
            gameOver();
            return;
        }
        else if(button[0].getText().equals(button[3].getText()) && button[3].getText().equals(button[6].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[0].getText());
            gameOver();
            return;
        }
        else if(button[1].getText().equals(button[4].getText()) && button[4].getText().equals(button[7].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[1].getText());
            gameOver();
            return;
        }
        else if(button[1].getText().equals(button[4].getText()) && button[4].getText().equals(button[7].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[1].getText());
            gameOver();
            return;
        }
        else if(button[2].getText().equals(button[5].getText()) && button[5].getText().equals(button[8].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[2].getText());
            gameOver();
            return;
        }
        else if(button[0].getText().equals(button[4].getText()) && button[4].getText().equals(button[8].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[0].getText());
            gameOver();
            return;
        }
        else if(button[2].getText().equals(button[4].getText()) && button[4].getText().equals(button[6].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[2].getText());
            gameOver();
            return;
        }

        int i;

        for(i=0;i<button.length;i++)
        {
            if(button[i].isEnabled())
            {
                break;
            }
        }

        if(i == button.length)
        {
            JOptionPane.showMessageDialog(null,"This was a Draw");
        }
    }

Start with replacing common code with function calls.
For example, you can create a method for checking if an entire row has the same text:

bool checkRow(int row)
{
    int col = row*3;
    return button[col].getText().equals(button[col+1].getText())
           && button[col+1].getText().equals(button[col+2].getText());
}

Write similar methods for columns and diagonal. Then you can do something like that:

bool checkWin()
{
    for(int i=0; i<3; i++)
    {
        if(checkRow(i))
            return true;
        if(checkCol(i))
            return true;
    }
    if(checkMajorDiag())
        return true;
    if(checkMinorDiag())
        return true;
    return false;
}

And then your entire if-else chain will be replaced with:

if(checkWin())
    {
        JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[2].getText());
        gameOver();
        return;
    }

The code is now much more readable.

Two major points you'd want to remember:

Duplicate code leads to bugs

Let's say you'd have to change the message printed to the user - when the message appears several times this task is tedious, plus in a real (big and complex) situation there's a good chance you'll forget to change one of the usages. In the new version, there's only one line to change.
Atomic operations are always better

When I reviewed your code, I couldn't tell immediately what each if condition means.

checkRow(0) means "check the first row". checkRow(0) is an atomic operation (atomic in this context means a single operation - a single function call).

button[0].getText().equals(button[1].getText()) && button[1].getText().equals(button[2].getText()) is much less understandable. But the real danger here that if the line was button[0].getText().equals(button[1].getText()) && button[2].getText().equals(button[2].getText()), you probably couldn't tell that one index was changed. Which means that debugging atomic operations is much easier.


What changes should I implement in my templated sorter class so that it performs efficiently? I have a college assignment to implement a class called sorter, whose object is a sorted vector (the user is allowed to sort it by the method he/she desires). I have been able to implement it as following. Please review it and suggest me the changes I should make to make it efficient.

template <typename T>
class Sorter {
    std::vector<T> vector;
    int count(T* array) {
        register int i = 0;
        while (*(array + i)) {
            i++;
        }
        i--;
        return i;
    }
    void swap(T& t1, T& t2) {
        T temp = t1;
        t1 = t2;
        t2 = temp;
    }
    void bubbleSort() {
        for (int i = 0; i < vector.size(); i++) {
            for (int j = 0; j < vector.size() - 1; j++) {
                if (vector[j] > vector[j + 1]) {
                    swap(vector[j], vector[j + 1]);
                }
            }
        }
    }
    std::vector<T> mergeSort(std::vector<T> vec) {
        int begin = 0;
        int end = vec.size() - 1;
        auto combine = [=](std::vector<T> lv, std::vector<T> rv) {
            std::vector<T> retVec;
            int i = 0;
            int j = 0;
            while (i < lv.capacity() && j < rv.capacity()) {
                if (lv[i] < rv[j]) {
                    retVec.push_back(lv[i]);
                    i++;
                }
                else {
                    retVec.push_back(rv[j]);
                    j++;
                }
            }
            while (i < lv.capacity()) {
                retVec.push_back(lv[i]);
                i++;
            }
            while (j < rv.capacity()) {
                retVec.push_back(rv[j]);
                j++;
            }
            return retVec;
        };
        if (vec.size() == 1) {
            return vec;
        }
        int mid = (begin + end) / 2;
        auto it = vec.begin();
        std::vector<T> left = mergeSort(std::vector<T>(it, it + mid + 1));
        std::vector<T> right = mergeSort(std::vector<T>(it + mid + 1, vec.end()));
        return combine(left, right);
    }
    void sort() {
        std::cout << "Choose your option:" << "\n";
        std::cout << "1. bubblesort" << "\n";
        std::cout << "2. mergesort" << "\n";
        int op = 2;
        //std::cin >> op;
        switch (op) {
        case 1:
            bubbleSort();
            break;
        case 2:
            vector = mergeSort(vector);
            break;
        }
    }
public:
    Sorter() = delete;
    Sorter(std::vector<T> vec) : vector(vec) {
        sort();
    }
    Sorter(T* array) {
        int num = count(array);
        for (int i = 0; i < num; i++) {
            this->vector.push_back(*(array + i));
        }
        sort();
    }
    std::vector<T> getSorted() {
        return vector;
    }
};

The updated version is always available at this link

In the sort() function its currently hardcoded to merge sort which I assume is not intentional. However, if you do allow the user to give an input you should do some error checking. You'll currently get unexpected behaviour if the user enters anything other than 1 or 2.
You should use std::swap rather than implementing your own swap function. Some types can make optimisations over what you've done.
From a design point of view I would argue that c++ is not an inherently object oriented language and that bubblesort and mergesort should be free functions taking a pair of iterators, to allow them to be more easily reused. You've pretty much done this for merge sort already, it uses none of the members of the Sorter class so could just be made a free function.
Your bubble sort implementation is inefficient. It should check if the vector is sorted after each pass, rather than doing the outer loops you could do while(!isSorted(vector)) or similar.
Merge sort has a very big lambda in it for combine. From a readability  point of view I would pull that out into a separate function.
Both mergesort and bubblesort use indices with operator[] to get the elements from the vector. The preferred way to do this in c++ is using iterators, see https://stackoverflow.com/questions/131241/why-use-iterators-instead-of-array-indices
In count() register is deprecated (and strictly is removed since c++17). You should probably avoid using it to improve compatibility.
I would need to think about it more, but I question if while (*(array + i)) in count will always behave as expected. You assume that the array has an end sentinel value that will dereference to null. I don't that's guaranteed. As far as I know there is not an easy way to get the size from a c style array. The normal way to handle this is to require the user to pass in the size as well.
Update on capacity(). Your use of capacity() in the merge sort is not correct. You should use size() instead. size() tells you the number of elements currently in the vector whereas capacity() tells you how many elements the vector reserved space for internally. i.e. if you insert more than this number of things into the vector it will internally reserve more space and do reallocate all the elements. It's important to note that the capacity can be greater than the number of elements in the vector. In your case it works ok as the constructor with a pair of iterators initialises the capacity equal to the size. However, I don't think this is guaranteed and is complier dependant. To see the difference try the following code:

    std::vector<int> v;
    std::cout << 0 << " " << v.size() << " " << v.capacity() <<"\n";
    for( int i = 0; i < 10; ++i)
    {
        v.push_back(i);
        std::cout << i << " " << v.size() << " " << v.capacity() <<"\n";
    }

0 0 0
0 1 1
1 2 2
2 3 4
3 4 4
4 5 8
5 6 8
6 7 8
7 8 8
8 9 16
9 10 16


Project Euler Problem 1 - Multiples of 3 and 5 I wrote this function for Project Euler Problem 1.

prob001 :: (Integral a) => [a] -> [a] -> a
prob001 a b = sum [x | x <- a, product ( map (x `rem`) b ) == 0]

The use is like this

GHCi> prob001 [1..999] [3, 5]
233168

But I am not at all satisfied with it. The list comprehension looks like something I have taken from Python. But I think the predicate can be much more efficient if it were written as a short-circuiting function. Currently it will have to evaluate each of these.

I am not able to think how to write that part in a short-circuiting way without a variable to hold the values. Am I correct in thinking that or not? Anything other that I can improve here?

I think that any is your friend here.

aMultipleOf factors x = any (\f -> x `rem` f == 0) factors
prob001 :: (Integral a) => [a] -> [a] -> a
prob001 nums factors = sum $ filter (aMultipleOf factors) nums


Conditionally set attributes in MongoDB based on form fields I have code which gets data from a POST request (request.form dict), compares with corresponding attribute in object accident and does some action. The code looks needlessly verbose.  How can I optimize it?

if accident.status == 1:
    if request.form.get('reason') \
        and request.form['reason'] != accident.reason:
        accident.set_reason(request.form['reason'], current_user)
    if request.form.get('note') \
        and request.form['note'] != accident.note:
        accident.note = request.form['note']
else:
    if request.form.get('priority') \
        and int(request.form['priority']) != accident.priority:
        accident.set_priority(current_user, int(request.form['priority']))
    if request.form.get('status') \
        and int(request.form['status']) != accident.status:
        accident.set_status(current_user, int(request.form['status']))
    if request.form.get('duration') \
        and int(request.form['duration']) != accident.duration:
        accident.duration = int(request.form['duration'])
    if request.form.get('service_status') \
        and int(request.form['service_status']) != accident.service_status:
        accident.set_service_status(int(request.form['service_status']))

Below is a stripped down version of Accident class, which is inconsistent because there are many fields, and sometimes setting a field requires many additional actions like logging or checking for user rights. The class derives from MongoEngine schema class.

class Accident(Document):
    priority = IntField(choices=PRIORITIES)
    status = IntField(choices=STATUSES, default=0)
    duration = IntField()
    region = StringField()
    service_status = IntField(choices=(0, 1, 2))
    reason = StringField(default=u"")
    note = StringField(default=u"")

    def set_priority(self, user=None, priority=None):
        self.priority = priority
        # Functions starting with `set_` do some activity that has
        # nothing to do with setting and validating a value. That's
        # why i don't use @property decorator. `to_history(user)` is
        # just an example.
        to_history(user)

    def set_status(self, user, status):
        self.status = status
        to_history(user)

    def set_reason(self, reason, user):
        self.reason = reason
        to_history(user)

    def set_service_status(self, ss):
        self.service_status = ss
        to_history(user)

If this is necessary, how can i improve class structure? It seems like I can't use @property decorator for setters, because my setters accept additional parameters. However I don't like the idea of writing a setter for all possible attributes. I use request.form.get(foo) instead of foo in request.form, because the dictionary may contain foo: None pair, and I don't want to accidentally set an attribute to None.

I'm not 100% sure there is a particularly elegant way to write this. If the code did not have a different body for each if statement, one could rewrite it like this

FORM_VALUES = {
    "priority": accident.priority,
    "status": accident.status,
    "duration": accident.duration,
    "service_status": accident.service_status
}

for key in FORM_VALUES:
    if request.form.get(key) and int(request.form[key]) != FORM_VALUES[key]:
        # As you use different functions for setting the values, this isn't possible
        request.set_value(key, request.form[key])

However, that is not the case, so don't get your hopes up. Regardless, there are still improvements to be made:

You're not checking that the header values are valid integers. I'd recommend either checking they are by using string.isdigit() or otherwise wrapping your calls to int() in a try except block.
You can use 'reason' in request.form instead of request.form.get('reason'); they have the same meaning in the places you've used them, but the first is easier to read.
It might be good if you used an enumeration for the values to Accident.status. As it is, checking that accident.status == 1 doesn't tell me anything; using a meaningful identifier might solve this. You can do this quite simply, by just assigning values to each identifier: PENDING, IN_PROGRESS, FINISHED = STATUSES. I've used some random names, and assumed that there are 3 possible statuses, change this to be accurate. Then in your function, instead of if accident.status == 1, we can have if accident.status == PENDING, which is a lot easier to understand without needing to know the meaning of each value of status.
In general, you should try and be consistent with the ordering of your arguments: your set_reason takes first a reason, then a user argument, while all of your other methods take the user argument first. The set_service_status doesn't take a user argument at all, which is fairly impressive, considering it uses a user variable in the function.

Overall not bad code. I don't agree with your comments on how @property would be inappropriate there, but it's far from the end of the world. In general, I'd try and be more careful to validate user input, but it may be that you do not need to worry about that.


Factory for arbitrarily high-order operations Grade 0: ++

This is so low that it is boring; the operation of grade 0 is incrementing or counting up, for example:

5++ -> 6
8++ -> 9

Grade 1: +

Addition is repeated incrementing:

4 + 3 -> 4++ ++ ++ -> 7
2 + 2 -> 2++ ++ -> 4

Grade 2: *

Multiplication is repeated addition:

3 * 4 -> 3 + 3 + 3 + 3 -> 12
5 * 2 -> 5 + 5 -> 10

...

I may go on but you understand that each operation of grade n + 1 is just a repetition of the operation of grade n, and that something like:

operation_of_order_100(4, 3)

is going to be unimaginably big, so big that it would be impossible to write it as a tower of exponents (exponentiation is a mere grade 3).

Even if computing an operation of order bigger than 5 is pratically impossible, I have gone through the mental exercise of writing a programme that raises the level of a given function and generates functions of a given grade, and I ask you for suggestions:

def increase_order(op):
    def increased(a, b):
        start_a = a
        for _ in range(b-1):
            a = op(a,start_a)
            #print(a)
        return a
    return increased

def add(a, b):
    return a + b

mul = increase_order(add)

assert mul(3,5) == 15
assert mul(7,3) == 21

exp = increase_order(mul)

assert exp(3,2) == 9
assert exp(7,2) == 49

tetract = increase_order(exp)

assert tetract(7,2) == 823543 # 7**7
assert tetract(4,3) == 4294967296 # 4**4**4

pentation = increase_order(tetract)

# print(pentation(4, 2)) :: Very big already

def op_of_order(order, start=add):
    for _ in range(order-1):
        start = increase_order(start)
    return start

BEAST_OPERATION = op_of_order(10**5)
# BEAST_OPERATION(2, 3) :: unimaginably big

Since this is tagged "functional-programming" I decided to take the functional approach. However, take note that Python isn't necessarily a "functional language," so it's not going to be as efficient as the iterative approach you were using. However, it looks like you're doing this from a theoretical standpoint, so you may find this interesting.

import functools

def iterate(f, x, n):
    """ Applies f n times to x. """
    if n == 0:
        return x
    else:
        return f(iterate(f, x, n - 1))

def iterate_for(f, x, n):
    """ Applies f n times to x, without recursion. """
    for _ in range(n):
        x = f(x)
    return x

def increase_order(f):
    return lambda x, n: iterate(functools.partial(f, x), x, n - 1)

def op_of_order(f, order):
    return iterate(increase_order, f, order)

def test():
    add = lambda x, y: x + y

    mul = increase_order(add)
    assert mul(3,5) == 15
    assert mul(7,3) == 21

    exp = increase_order(mul)
    assert exp(3,2) == 9
    assert exp(7,2) == 49

    tetract = op_of_order(add, 3)
    assert tetract(5,2) == 3125
    #assert tetract(7,2) == 823543
    #assert tetract(4,3) == 4294967296

if __name__ == "__main__":
    test()

The biggest change here was the abstraction of the loop you have in both increase_order and op_of_order into a function called iterate, inspired by Haskell's iterate function. I've provided both a recursive and non-recursive version for understanding and efficiency (Python has a no tail-call optimization) during testing.

In my opinion, both increase_order and op_of_order are much clearer when written this way, making it easier to see just what both functions actually do.


Calculating the price of digital media I have taken over maintenance of some code, and have restructured it after reading some documents about PSR-2:

<?php

function calc_p()
{
    list($arg1, $arg2, $arg3) = func_get_args();
    switch($arg1) {
      case 'ebook':
            $bp = 5;
            $tax = .08;
              break;
      case 'audio_book':
           $bp = 5;
           $tax = .08;
              break;
      case 'text_book':
           $bp = 7.5;
           $tax = .1;
           break;
    }
    $bp = ($bp + ($bp*$tax));
    //Calclate shipping
    if($arg3 == 'California') {
    }
    else {
        $bp = $bp + 1;
    }
    if ($arg3 == 'Texas') {
        $norm = $bp;
        $bp = $norm + 5;
    }
    //final price format: $XX:XX
    $final_price = '$'.$bp;
    if (strpos($final_price, '.')) {
        $num = (2 - (strlen(substr($final_price, strpos($final_price, '.') + 1))));

            for ($num>0; $num--;) {
                $final_price .= '0';
            }
    }
    elseif (strpos($final_price, '.') == FALSE) {
        $final_price = $final_price .'.00';
    }
    else {
    }
    return $final_price;
}
$totalprice = calc_p('text_book','oldversion','Texas');
echo '<p style="font-weight: bold;">Total Price: </p>' . $totalprice;
?>

I wanted to ask if what I've done is already enough to follow the coding standards, or if I am missing something.

I've been waiting for this question to be reopened because the formatting issues here are quite extraordinary! I think I'll keep this post to a clean code/code styling post exclusively, that way others can touch on purpose and implementations.

I think what's required here is a bullet point list, which I know is boring, but it's the easiest way to go line by line! Let's begin :)

Eeek! What in the world is calc_p()!? I immediately know nothing about your code. That's the opposite of what we want. Names should be meaningful and thorough. It looks like you're calculating a price, so a better name might be calculateBookPrice(). Functions are usually expected to be camelCase.
$arg1: bad name. $arg2: bad name. $arg3: bad name. What are these variables? We need description in these names!
I had no idea this function needed variables for func_get_args(). You limit the arguments to three, so I'd say add three parameters to the function. That or have something such as ... $someArguments for a parameter. (Obviously with a name that fits your code)
What is $bp? I immediately have converted this name to "blood pressure". However, that doesn't make sense. Now your reader is confused. See how important useful variable names are?
'ebook', 5, .08: what do all these have in common? I don't know why you chose them. If you need to, create (constants, if you have a class) variables and apply these values to a named variable.
I don't see a default to your switch. I need a default to function! It's best practice to use one anyways.
What in the world is $bp = ($bp + ($bp*$tax));? Hows about $bp *= ++$tax;? It's shorter and takes advantage PHP's combined operators and pre-increment operator. However, even this isn't too pretty. Perhaps refactor the code before it...
//Calclate shipping - this is a useless comment. Comment's should describe why.
It'd be nice to get some place in between statements: if(, '$'.$bp.
The condition for $arg3 == 'California' is empty, why not negate the comparison? ($arg3 != 'California')
$bp = $bp + 1 --> $bp++
Shouldn't you have a switch for the states you check against?
What is $norm? Are you referring to a person named Norman? Spell out a useful variable name!
You may want to look into money_format or formatCurrency... That whole block is hideous!
Some variables are like $final_price and some are like $totalprice! It's vital that you stay consistent!


Drawing inverted triangles I was working through another question here and have hacked up an answer:

def get_rows():
""" Get height of triangle from user"""

    while True:
        rows = input('Enter the number of rows:  ')
        if 3 <= rows <= 33:
            return rows

def triangle(rows, invert=True):
""" Print the outline of a triangle, of variable size.
    Print out a regular or inverted version of the triangle as necessary """

    if invert:
        height = -1 * rows
    else:
        height = 0

    # inner padding for min height (3)
    inner_buffer = [0, 1, 3]
    while len(inner_buffer) <= rows:
        inner_buffer.append(inner_buffer[-1]+2)

    level = 0
    while level <= rows:
        outer_padding = ' '*(rows - abs(height))

        if height == 0:
            print(outer_padding + '*')
        else:
            inner_padding = ' '*( inner_buffer[ abs(height) ] )
            print(outer_padding + '*' + inner_padding + '*')

        height += 1
        level += 1

At this stage I am working to re-implement this two ways using: recursion and itertools.

But I am looking at this and wonder if it would be a good place to use decorators. If someone is happy to implement one triangle (either regular or inverted, I don't mind) I would love to see the difference in the 3 implementations.

Decorators are a tool that you use when you have many functions that share a behaviour, for example a particular calling convention or treatment of results. See the Python Decorator Library on the Python wiki for some examples, and this answer on Stack Overflow for a tutorial.

Here you only have two functions, so it doesn't seem likely that decorators will be helpful. Use the right tool for the job!

Anyway, comments on your code.

No docstrings! What do these functions do and how do you call them?
The else: continue in get_rows() is unnecessary.
The regular keyword argument to triangle is ignored.
Your function triangle seems very long for what it's doing. Why not use Python's format language to do the padding? Like this:

def triangle(n, invert=True):
    """Print inverted triangle of asterisks (with `n`+1 rows) to standard output.
    Print it the right way up if keyword argument `invert=False` is given.
    """
    for i in reversed(xrange(n + 1)) if invert else xrange(n + 1):
        print('{:>{}}{:>{}}'.format('*', n - i + 1, '*' if i else '', 2 * i))


Switch like context manager in Python This is self-explaining example with usage in doctests (it's not that fast as implementation with dict key-lookups, but it's a lot more readable, and don't require callables and lambdas):

class Switch(object):
    """
    Switch, simple implementation of switch statement for Python, eg:

    >>> def test_switch(val):
    ...   ret = []
    ...   with Switch(val) as case:
    ...     if case(1, fall_through=True):
    ...       ret.append(1)
    ...     if case(2):
    ...       ret.append(2)
    ...     if case.call(lambda v: 2 < v < 4):
    ...       ret.append(3)
    ...     if case.call(lambda v: 3 < v < 5, fall_through=True):
    ...       ret.append(4)
    ...     if case(5):
    ...       ret.append(5)
    ...     if case.default:
    ...       ret.append(6)
    ...   return ret
    ...
    >>> test_switch(1)
    [1, 2]

    >>> test_switch(2)
    [2]

    >>> test_switch(3)
    [3]

    >>> test_switch(4)
    [4, 5]

    >>> test_switch(5)
    [5]

    >>> test_switch(7)
    [6]

    >>> def test_switch_default_fall_through(val):
    ...   ret = []
    ...   with Switch(val, fall_through=True) as case:
    ...     if case(1):
    ...       ret.append(1)
    ...     if case(2):
    ...       ret.append(2)
    ...     if case.call(lambda v: 2 < v < 4):
    ...       ret.append(3)
    ...     if case.call(lambda v: 3 < v < 5, fall_through=False):
    ...       ret.append(4)
    ...     if case(5):
    ...       ret.append(5)
    ...     if case.default:
    ...       ret.append(6)
    ...   return ret
    ...
    >>> test_switch_default_fall_through(1)
    [1, 2, 3, 4]

    >>> test_switch_default_fall_through(2)
    [2, 3, 4]

    >>> test_switch_default_fall_through(3)
    [3, 4]

    >>> test_switch_default_fall_through(4)
    [4]

    >>> test_switch_default_fall_through(5)
    [5]

    >>> test_switch_default_fall_through(7)
    [6]
    """

    class StopExecution(Exception):
        pass

    def __init__(self, test_value, fall_through=False):
        self._value = test_value
        self._fall_through = None
        self._default_fall_through = fall_through
        self._use_default = True
        self._default_used = False

    def __enter__(self):
        return self

    def __exit__(self, exc_type, exc_val, exc_tb):
        if exc_type is self.StopExecution:
            return True

        return False

    def __call__(self, expr, fall_through=None):
        return self.call(lambda v: v == expr, fall_through)

    def call(self, call, fall_through=None):
        if self._default_used:
            raise SyntaxError('Case after default is prohibited')

        if self._finished:
            raise self.StopExecution()
        elif call(self._value) or self._fall_through:
            self._use_default = False
            if fall_through is None:
                self._fall_through = self._default_fall_through
            else:
                self._fall_through = fall_through
            return True

        return False

    @property
    def default(self):
        if self._finished:
            raise self.StopExecution()

        self._default_used = True

        if self._use_default:
            return True

        return False

    @property
    def _finished(self):
        return self._use_default is False and self._fall_through is False

class CSwitch(Switch):
    """
    CSwitch is a shortcut to call Switch(test_value, fall_through=True)
    """
    def __init__(self, test_value):
        super(CSwitch, self).__init__(test_value, fall_through=True)

I like your trick to create this syntactic sugar.  The implementation is also pretty good, as are the doctests.

Feature suggestions

I think it would be nice if a case() could test for multiple values.   A case('jack', 'queen', 'king') should match if the Switch was created with any of those three strings.

It would also be nice if there were a case.match() that performed a regular expression match.

Minor issues

If execution ends up inside case.call() due to fall-through, then I would expect the test function not to be called at all, as a kind of short-circuiting behaviour.  Specifically,

elif call(self._value) or self._fall_through:

should be reversed and written as

elif self._fall_through or call(self._value):

Having a parameter named call when the method is also named call is confusing.  I suggest renaming the parameter to test.
Avoid testing variables for equality with True and False explicitly.  Just use boolean expressions.  For example, in __exit__(), change

def __exit__(exc_type, exc_val, exc_tb):
    if exc_type is self.StopExecution:
        return True
    return False

to

def __exit__(exc_type, exc_val, exc_tb):
    return exc_type is self.StopExecution

Initialize _fall_through to False instead of None; it's slightly more informative.
Rename expr → case_value.  Rename _value → _switch_value.
Rename / invert _use_default to not _matched_case, because _use_default is too confusingly similar to _default_used.  Also, by inverting the logic, all three private variables can be initialized to False, which is more elegant.
Instead of a _finished property, write a _check_finished() method that raises StopException too.

Proposed solution

import re

class Switch(object):
    """
    Switch, simple implementation of switch statement for Python, eg:

    >>> def test_switch(val):
    ...   ret = []
    ...   with Switch(val) as case:
    ...     if case(1, fall_through=True):
    ...       ret.append(1)
    ...     if case.match('2|two'):
    ...       ret.append(2)
    ...     if case.call(lambda v: 2 < v < 4):
    ...       ret.append(3)
    ...     if case.call(lambda v: 3 < v < 5, fall_through=True):
    ...       ret.append(4)
    ...     if case(5, 10):
    ...       ret.append('5 or 10')
    ...     if case.default:
    ...       ret.append(6)
    ...   return ret
    ...
    >>> test_switch(1)
    [1, 2]

    >>> test_switch(2)
    [2]

    >>> test_switch(3)
    [3]

    >>> test_switch(4)
    [4, '5 or 10']

    >>> test_switch(5)
    ['5 or 10']

    >>> test_switch(10)
    ['5 or 10']

    >>> test_switch(7)
    [6]

    >>> def test_switch_default_fall_through(val):
    ...   ret = []
    ...   with Switch(val, fall_through=True) as case:
    ...     if case(1):
    ...       ret.append(1)
    ...     if case(2):
    ...       ret.append(2)
    ...     if case.call(lambda v: 2 < v < 4):
    ...       ret.append(3)
    ...     if case.call(lambda v: 3 < v < 5, fall_through=False):
    ...       ret.append(4)
    ...     if case(5):
    ...       ret.append(5)
    ...     if case.default:
    ...       ret.append(6)
    ...   return ret
    ...
    >>> test_switch_default_fall_through(1)
    [1, 2, 3, 4]

    >>> test_switch_default_fall_through(2)
    [2, 3, 4]

    >>> test_switch_default_fall_through(3)
    [3, 4]

    >>> test_switch_default_fall_through(4)
    [4]

    >>> test_switch_default_fall_through(5)
    [5]

    >>> test_switch_default_fall_through(7)
    [6]
    """

    class StopExecution(Exception):
        pass

    def __init__(self, switch_value, fall_through=False):
        self._switch_value = switch_value
        self._default_fall_through = fall_through
        self._fall_through = False
        self._matched_case = False
        self._default_used = False

    def __enter__(self):
        return self

    def __exit__(self, exc_type, exc_val, exc_tb):
        return exc_type is self.StopExecution

    def __call__(self, case_value, *case_values, **kwargs):
        def test(switch_value):
            return any(switch_value == v for v in (case_value,) + case_values)
        return self.call(test, **kwargs)

    def call(self, test, fall_through=None):
        if self._default_used:
            raise SyntaxError('Case after default is prohibited')

        self._check_finished()
        if self._fall_through or test(self._switch_value):
            self._matched_case = True
            self._fall_through = fall_through if fall_through is not None else self._default_fall_through
            return True

        return False

    def match(self, regex, fall_through=None):
        if self._default_used:
            raise SyntaxError('Match after default is prohibited')

        self._check_finished()
        if isinstance(regex, str):
            regex = re.compile(regex)
        if self._fall_through or regex.match(str(self._switch_value)):
            self._matched_case = True
            self._fall_through = fall_through if fall_through is not None else self._default_fall_through
            return True

        return False

    @property
    def default(self):
        self._check_finished()
        self._default_used = True
        return not self._matched_case

    def _check_finished(self):
        if self._matched_case and not self._fall_through:
            raise self.StopExecution()

class CSwitch(Switch):
    """
    CSwitch is a shortcut to call Switch(switch_value, fall_through=True)
    """
    def __init__(self, switch_value):
        super(CSwitch, self).__init__(switch_value, fall_through=True)


Copy file using read and write streams I have this piece of code in my program, where I assign 10000 as the size of the buffer. Now How can I check if the file that I am copying is less than 10000, for example 5000, than how to free the remaining 5000 back?
Thank You.
private void copyFile(File in, File out) {
        var buffer = new byte[10000];
        try {
            FileInputStream dis = new FileInputStream(in);
            FileOutputStream dos = new FileOutputStream(out);
            int count;
            do {
                count = dis.read(buffer);
                if (count != -1)
                    dos.write(buffer, 0, count);
            } while (count != -1);
            dis.close();
            dos.close();
        } catch (IOException e) {
            program.print("Error copying file: " + e.toString() + "\n", null);
        }
    }

EDIT: Is there anyway, I can optimize this more?

Streams should always be closed. You will have resource leaks if your method throws an exception. Either close the streams in a finally block, or use try-with-resources to make sure they get closed.
Optional curly braces aren't. Skipping them will often lead to bugs when code gets modified later.
The typical loop structure used for working with streams looks more like
int charsRead;
while ((charsRead = dis.read(buffer) > 0) {
    // do stuff
}

10,000 is pretty big for a buffer. That may not be the best choice.
Please don't swallow the stack trace. That's critical information when this code needs to be debugged later.
Opinion: var is icky.
If you apply all these changes, your code might look more like:
private void copyFile(File in, File out) {
    byte[] buffer = new byte[1024];
    try (FileInputStream dis = new FileInputStream(in);
            FileOutputStream dos = new FileOutputStream(out)) {
        int charsRead;
        while ((buffer = dis.read(buffer) > 0) {
                dos.write(buffer, 0, charsRead);
        }
    } catch (IOException e) {
        //I don't know how to use your logging library to display exception stack trace, so..
        e.printStackTrace(System.err);
    }
}

Of course, this is assuming that you can't use the rather intuitively named Files.copy() instead. That would be much better.
private void copyFile(File in, File out) {
    try {
        Files.copy(in.toPath(), out.toPath());
    } catch (IOException e) {
        e.printStackTrace(System.err);
    }
}


Simple Linux pipeline I've compiled a simple Linux pipeline and using my C style and modularization that I will use for my own operating system. Do you have something to say about this code (partly taken from SO)? For example, are all my imports necessary? How can I improve error handling?

#include <sys/types.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
struct command
{
    const char **argv;
};

int spawn_proc (int in, int out, struct command *cmd) {
    pid_t pid;
    if ((pid = fork ()) == 0) {
        if (in != 0) {
            dup2 (in, 0);
            close (in);
        }
        if (out != 1) {
            dup2 (out, 1);
            close (out);
        }
        return execvp (cmd->argv [0], (char * const *)cmd->argv);
    }
    return pid;
}

int fork_pipes (int n, struct command *cmd) {
    int i;
    pid_t pid;
    int in, fd [2];
    in = 0;
    for (i = 0; i < n - 1; ++i) {
        pipe (fd);
        spawn_proc (in, fd [1], cmd + i);
        close (fd [1]);
        in = fd [0];
    }
    if (in != 0)
        dup2 (in, 0);
    return execvp (cmd [i].argv [0], (char * const *)cmd [i].argv);
}

int main (int argc, char ** argv) {
    int i;
    if (argc == 1) { /* There were no arguments */
        const char *printenv[] = { "printenv", 0};
        const char *sort[] = { "sort", 0 };
        const char *less[] = { "less", 0 };
        struct command cmd [] = { {printenv}, {sort}, {less} };
        return fork_pipes (3, cmd);
    }
    if (argc > 1) { /* I'd like an argument */
        char *tmp;
        int len = 1;
        for( i=1; i<argc; i++)
        {
            len += strlen(argv[i]) + 2;
        }
        tmp = (char*) malloc(len);
        tmp[0] = '\0';
        int pos = 0;
        for( i=1; i<argc; i++)
        {
            pos += sprintf(tmp+pos, "%s%s", (i==1?"":"|"), argv[i]);
        }
        const char *printenv[] = { "printenv", 0};
        const char *grep[] = { "grep", "-E", tmp, NULL};
        const char *sort[] = { "sort", 0 };
        const char *less[] = { "less", 0 };
        struct command cmd [] = { {printenv}, {grep}, {sort}, {less} };
        return fork_pipes (4, cmd);
        free(tmp);
    }
}

What it does is simply a printenv | sort | less if there are no args and if there are arguments it is taken as parameter list to grep to grep for environment varibles. I'm using it to develop my programming and make my own command-line environment for my own FPGA operating system.

dup2 handles equal file descriptors very nicely. There is no need to test for in != 0 etc.
On the other hand, you must check the return values, especially those of the system calls, and give a proper diagnostics when necessary.
An idea of executing the last pipeline component inside a master process is dubious. You lose the control over children's lifecycle. I recommend to run each component in its own process, and wait for all of them.

An i < n - 1 loop terminating condition looks alarming. It takes some mental effort to understand what you are after. You may try to heal it with the comment, but most likely you should treat all pipeline components equally.
Keep in mind that some commands (cd being most prominent) must be builtins. Spawning a shell for them would be a design mistake.


VersionString (eg "1.0.2") IComparer algorithm For a project I'm working on, I need to compare version strings where:

version strings are composed only of numbers and periods
version strings are made up of an arbitrary number of segments (major.minor, manjor.minor.build, etc)
1.0 is equivalent to 1.0.0, etc.

Please provide feedback on performance considerations or style.

Here's my shot at it:

public class VersionStringComparer : IComparer<string>
{
    public int Compare(string x, string y)
    {
        if (x == y) return 0;

        var xparts = x.Split('.');
        var yparts = y.Split('.');

        var length = new[] {xparts.Length, yparts.Length}.Max();

        for (var i = 0; i < length; i++)
        {
            int xint;
            int yint;

            if (!Int32.TryParse(xparts.ElementAtOrDefault(i), out xint)) xint = 0;
            if (!Int32.TryParse(yparts.ElementAtOrDefault(i), out yint)) yint = 0;

            if (xint > yint) return 1;
            if (yint > xint) return -1;
        }

        //they're equal value but not equal strings, eg 1 and 1.0
        return 0;
    }
}

And two quick unit tests (using the Should.Fluent library:

    [TestMethod]
    public void VersionStringComparerSortsMajorAndMinorVersionString()
    {
        var versions = new[]{"1.5","1.0","2.0","1.0"};
        var versionStringComparer = new ReleaseGateway.Util.VersionStringComparer();

        var sorted = versions.OrderBy(x => x, versionStringComparer).ToArray();

        var expected = new[] {"1.0", "1.0", "1.5", "2.0"};
        sorted.Should().Equal(expected);
    }

    [TestMethod]
    public void VersionStringComparerSortsArbitraryRadixVersionStrings()
    {
        var versions = new[] { "1.5.4", "1.7", "1.0", "1","1.4" };
        var versionStringComparer = new ReleaseGateway.Util.VersionStringComparer();

        var sorted = versions.OrderBy(x => x, versionStringComparer).ToArray();

        var expected = new[] { "1.0", "1", "1.4", "1.5.4", "1.7" };
        sorted.Should().Equal(expected);
    }

I adjusted the compare method slightly, making it more readable (IMO) through the use of an Anonymous type with named properties and by extracting two helper methods.

I'm not checking the result of TryParse anymore since the result is 0 for failed conversions.
Regarding performance: on my machine, your version runs ten thousand iterations in ~400ms, while mine seems to shave off around one fourth, running the same amount of iterations at ~300ms.

This should be because I moved the parsing of the versions out of the loop.
I measured this using a System.Diagnostics.Stopwatch.

EDIT: A note on performance... micro-optimizations such as the ones I've done are meaningless unless you have actually identified a real bottleneck in your code. I only ran these simplified benchmarks to make sure I wasn't making your implementation slower while getting it into a more expressive form.

public int Compare(string x, string y)
{
    if (x == y) return 0;
    var version = new { First = GetVersion(x), Second = GetVersion(y) };
    int limit = Math.Max(version.First.Length, version.Second.Length);
    for (int i = 0; i < limit; i++)
    {
        int first = version.First.ElementAtOrDefault(i);
        int second = version.Second.ElementAtOrDefault(i);
        if (first > second) return 1;
        if (second > first) return -1;
    }
    return 0;
}

private int[] GetVersion(string version)
{
    return (from part in version.Split('.')
            select Parse(part)).ToArray();
}

private int Parse(string version)
{
    int result;
    int.TryParse(version, out result);
    return result;
}

This adapted converter returns the same test results as your original one - though I haven't tried to test it more thoroughly - maybe someone can spot an edge case? ;-)


Convenience function for writing files in CSV, JSON, etc The McCabe code complexity of the following function is 19 as found by pytest --mccabe test.py (from my toy-project mpu). While I agree that this function is length and likely will become longer in future, I don't know how to improve the situation.

What it does

The code mpu.io.write(filepath) is a convenience function for writing a few common file formats.

Code

This code should work in Python 2 and 3.

def write(filepath, data, **kwargs):
    """
    Write a file.
    Supported formats:
    * CSV
    * JSON, JSONL
    * pickle
    Parameters
    ----------
    filepath : str
        Path to the file that should be read. This methods action depends
        mainly on the file extension.
    data : dict or list
        Content that should be written
    kwargs : dict
        Any keywords for the specific file format.
    Returns
    -------
    data : str or bytes
    """
    if filepath.lower().endswith('.csv'):
        kwargs_open = {'newline': ''}
        mode = 'w'
        if sys.version_info < (3, 0):
            kwargs_open.pop('newline', None)
            mode = 'wb'
        with open(filepath, mode, **kwargs_open) as fp:
            if 'delimiter' not in kwargs:
                kwargs['delimiter'] = ','
            if 'quotechar' not in kwargs:
                kwargs['quotechar'] = '"'
            with open(filepath, 'w') as fp:
                writer = csv.writer(fp, **kwargs)
                writer.writerows(data)
        return data
    elif filepath.lower().endswith('.json'):
        with io_stl.open(filepath, 'w', encoding='utf8') as outfile:
            if 'indent' not in kwargs:
                kwargs['indent'] = 4
            if 'sort_keys' not in kwargs:
                kwargs['sort_keys'] = True
            if 'separators' not in kwargs:
                kwargs['separators'] = (',', ': ')
            if 'ensure_ascii' not in kwargs:
                kwargs['ensure_ascii'] = False
            str_ = json.dumps(data, **kwargs)
            outfile.write(to_unicode(str_))
    elif filepath.lower().endswith('.jsonl'):
        print(filepath)
        with io_stl.open(filepath, 'w', encoding='utf8') as outfile:
            kwargs['indent'] = None  # JSON has to be on one line!
            if 'sort_keys' not in kwargs:
                kwargs['sort_keys'] = True
            if 'separators' not in kwargs:
                kwargs['separators'] = (',', ': ')
            if 'ensure_ascii' not in kwargs:
                kwargs['ensure_ascii'] = False
            for line in data:
                str_ = json.dumps(line, **kwargs)
                outfile.write(to_unicode(str_))
                outfile.write(u'\n')
    elif filepath.lower().endswith('.pickle'):
        if 'protocol' not in kwargs:
            kwargs['protocol'] = pickle.HIGHEST_PROTOCOL
        with open(filepath, 'wb') as handle:
            pickle.dump(data, handle, **kwargs)
    elif (filepath.lower().endswith('.yml') or
          filepath.lower().endswith('.yaml')):
        raise NotImplementedError('YAML is not supported, because you need '
                                  'PyYAML in Python3. '
                                  'See '
                                  'https://stackoverflow.com/a/42054860/562769'
                                  ' as a guide how to use it.')
    elif (filepath.lower().endswith('.h5') or
          filepath.lower().endswith('.hdf5')):
        raise NotImplementedError('HDF5 is not supported. See '
                                  'https://stackoverflow.com/a/41586571/562769'
                                  ' as a guide how to use it.')
    else:
        raise NotImplementedError('File \'{}\' is not known.'.format(filepath))

You’ve got your code split into various sections; why not make the sections their own methods?

if filepath.lower().endswith('.csv'):
   return _write_csv(filepath, data, kwargs)
elif filepath.lower().endswith('.json'):
   return _write_json(filepath, data, kwargs)
elif filepath.lower().endswith('.jsonl'):
   return _write_jsonl(filepath, data, kwargs)
elif filepath.lower().endswith('.pickle'):
   return _write_pickle(filepath, data, kwargs)
elif filepath.lower().endswith('.yml') or
     filepath.lower().endswith('.yaml'):
   return _write_yml(filepath, data, kwargs)
elif filepath.lower().endswith('.h5') or
     filepath.lower().endswith('.hdf5'):
   return _write_hdf5(filepath, data, kwargs)
else:
   raise NotImplementedError('File \'{}\' is not known.'.format(filepath))

With the appropriate dict, you could even:

path = filepath.lower()
for ending, writer in writers.items():
    if path.endswith(ending):
        return writer(filepath, data, kwargs)
raise NotImplementedError(f"File {filepath} is not known")

Or,

ext = os.path.splitext(filepath)[1].lower()
if ext in writers:
    writers[ext](filepath, data, kwargs)
raise NotImplementedError(f"File {filepath} is not known")


A basic form with React I have been working of a basic from using React and I need your help to improve this part of my code, especially validate and onSubmit.

state = {
    email: "",
    emailError: "",
  };
  change = e => {
    this.setState({
      [e.target.name]: e.target.value
    });
  };

    validate = () => {
    let isError = false;
     const errors = {
      emailError: "",
    };

    if (this.state.email.indexOf("@") === -1) {
      isError = true;
      errors.emailError = "Requires valid email";
    }

    this.setState({
      ...this.state,
      ...errors
    });

    return isError;
  };
  onSubmit = e => {
    e.preventDefault();
    const err = this.validate();
    if (!err) {
      this.props.onSubmit(this.state);
      this.setState({

        email: "",
        emailError: "",
      });
    }
  };

  render() {
    return (
      <form>
        <TextField
          name="email"
          hintText="Email"
          floatingLabelText="Email"
          value={this.state.email}
          onChange={e => this.change(e)}
          errorText={this.state.emailError}
          floatingLabelFixed
        />
        <br />
        <RaisedButton label="Submit" onClick={e => this.onSubmit(e)} primary />
      </form>
    );

As far as I'm aware, CodeReview is for completed programs. I imagine that this question would be better suited in StackOverflow. That being said, I see no point in duping the question on a different site.

Without seeing the rest of the program, and solely in a time interest, I'd say you keep the change function and ignore the validations. If you want a form field to be required, you can add 'required' to the input.

<form>
  <input type="text" name="input1" value={this.state.input1} onChange={e => this.change(e)} required />
</form>

You can find more on the MDN web docs for the required attribute.

Side note: you can also bind this.change as this.change = this.change.bind(this) in the constructor if you intend on using it for multiple fields. It'll save you some bytes, but if it's just a one off, binding it in the element itself like you have should be fine.


Pure bash program for auto-filling a template file with ENV variables Goal of Program

Consider a template file template.txt with double brace variables, intended to be replaced by values:

hello there {{ MY_VAR1 }}
some other stuff
some other stuff
foo: {{ MY_VAR2 }}
{{ MY_VAR2 }} is the value of MY_VAR2

and assume you have defined and exported those variables:

export MY_VAR1=val1
export MY_VAR2=val2

we want a script fill_template such that fill_template template.txt produces:

hello there val1
some other stuff
some other stuff
foo: val2
val2 is the value of MY_VAR2

and which gives an appropriate error message if any of the required template variables are not defined.

Code for review

Here is working code for fill_template:

#!/bin/bash

if [[ ! -f $1 ]]; then
  >&2 echo "Usage: $0 <filename>"
  exit 1
fi

# Gather all the required template variables

vars=()
while IFS= read -r line; do
    vars+=( "$line" )
done < <( awk 'match($0, /{{ (.*) }}/, a) { print a[1] }' "$1" | sort -u )

# Verify that all template variables are set and exported

missing=()
for var in "${vars[@]}"; do
  if [[ -z ${!var+x} ]]; then
    missing+=( "$var" )
  fi
done

if [[ ${#missing[@]} -gt 0 ]]; then
  >&2 echo "The following required variables have not been set and exported:"
  for var in "${missing[@]}"; do
    >&2 echo "${var}"
  done
  exit 1
fi

# Dynamically construct the sed cmd to do the replacement

sed_cmd=
for var in "${vars[@]}"; do
  sed_cmd+="s/\\{\\{ *${var} *}}/${!var}/g;"
done

sed -E "${sed_cmd}" "$1"

Notes

All comments welcome, from the high-level approach to nitpicks.
Currently we assume there will be only one {{ TEMPLATE_VAR }} per line

Make the template variable name extraction more strict

The Awk command extracts the template variable names using the pattern /{{ (.*) }}/.
This leaves some room to human errors.
For example excess whitespace, as in {{  MY_VAR   }}.
This can be especially frustrating for trailing whitespace, which will be invisible when printing the list of missing variables.
I think it would be good to strip whitespaces after {{ and before }}.

But that's not quite enough. Consider such line in the input:

hello there {{ MY_VAR1 }} foo bar {{ baz }}

Although using multiple variables is explicitly unsupported,
this blows up in the face of the user in a nasty way:

$ MY_VAR1=foo/bar/bazo MY_VAR2=bar bash script.sh input.txt
a.sh: line 19: MY_VAR1 }} foo bar {{ baz: bad substitution
a.sh: line 33: MY_VAR1 }} foo bar {{ baz: bad substitution
hello there {{ MY_VAR1 }} foo bar {{ baz }}
some other stuff
some other stuff
foo: {{ MY_VAR2 }}
{{ MY_VAR2 }} is the value of MY_VAR2

The error messages are unfortunately incomprehensible.

Since the template variable names are expected to take values from shell variables,
it would make sense to enforce a stricter pattern.

Even if the script is not intended to handle sophisticated scenarios,
I think it should handle such user mistakes more gracefully.

Consistency

The Sed command replacing template variable names with values uses the pattern \\{\\{ *${var} *}}.
This is not consistent with the one in the Awk command,
because of stripping the whitespace.
As mentioned earlier, I would adjust the Awk command to make it consistent.

Error handling

In the example above with a user mistake,
the script continued to execute even after the error.
To catch such issues and terminate the program early I recommend adding this line at the very beginning:

set -euo pipefail

Beware of some gotchas

/ in the template variable names and values will break the Sed command.

As for the names, a more strict handling as mentioned earlier will prevent this issue.

As for the values, / appearing in the values doesn't sound too crazy,
because I can easily imagine wanting to insert path strings.
So I think it's a legitimate concern that would be good to address.

Usability

The limitation to one template variable per line seems a bit artificial.

Currently the script fails fast when the user mistakenly tries to use multiple per line, that's a good behavior to preserve.
(I'm pointing this out because if you simply enforce more stricter checking on the name patterns, this fail-fast behavior may no longer be the case.
And if I had to choose between cryptic failures, and quietly ignored missed template variables, I would prefer cryptic failures.)


C: Approxmation of Pi with stratified sampling #include<stdio.h>
#include<stdlib.h>
#include<time.h>
#include<math.h>

int m, inbound, g;
double pi;
double uncertainty;
double pi_exactly=3.14159265358979323846;

int main(void)
{
    srand(time(NULL));

    printf("Quantity of random points per square: \n");
    scanf("%d", &m);

    printf("Gridsize: \n");
    scanf("%d", &g);

    for(int i=0; i<g; i++)
    {
        for(int j=0; j<g; ++j)
        {

            if(pow((i+1) / g,2)+pow((j+1) / g,2) < 1){
                inbound+=m;
            }else if(pow(i,2)+pow(j,2)>1){
                break;
            }else{

                for(int k=1; k<=m; k++)
                {
                    double x = ((double)rand() / RAND_MAX + i) * 1 / (double) g;
                    double y = ((double)rand() / RAND_MAX + j) * 1 / (double) g;

                    if(pow(x,2)+pow(y,2)<=1){
                        inbound+=k;
                    }
                }
            }
        }
    }

    pi = (double) inbound / ((double) m *  pow(g,2))*4;
    uncertainty = pi - pi_exactly;

    printf("The approximation of pi is: %lf\n", pi);
    printf("Uncertainty: %e\n", uncertainty);
return 0;

}

I want to approximate \$\pi\$ with the Monte-Carlo-Simulation, so if the random points are inside the unit circle, then there are "inbound". Otherwise not. Pi can be randomize with the divison of the inbound points and the whole amount of random points. Unfortunately the code does not approximate \$\pi\$ very precisely. With \$m=100, g=1000\$ I get \$\pi = 3.991...\$ Is there something to improve? Thanks!

Bug #1

In your first test:

        if(pow((i+1) / g,2)+pow((j+1) / g,2) < 1){

you didn't convert (i+1)/g to a double correctly.  You are using integer math, so (i+1)/g is going to be zero until i reaches g-1.  What you wanted to do was this:

        if(pow((double)(i+1) / g,2)+pow((double)(j+1) / g,2) < 1){

Bug #2

In your second test:

        }else if(pow(i,2)+pow(j,2)>1){

you forgot to divide by g.  So it should have been:

        }else if(pow((double)i/g,2)+pow((double)j/g,2)>1){

Bug #3

Your last test was OK, but you added the wrong amount to inbound:

                if(pow(x,2)+pow(y,2)<=1){
                    inbound+=k;
                }

Here, it should have been:

                if(pow(x,2)+pow(y,2)<=1){
                    inbound++;
                }

Otherwise, later when you divide by the number of samples, the math won't work out right.

Potential overflow

When I used 10000 points and a grid size of 1000, the calculation overflowed because inbound exceeded the maximum integer value.  You might want to use a 64-bit value instead of an int.

pow(x,2)

I have a pet peeve against writing pow(x,2).  I think that x*x is both clearer and faster.  Check my profile and you'll see.

Rewrite

Here is a rewrite of your program with the following additional changes:

Sped up the inner loop by multiplying against a precomputed value in order to avoid costly divisions.
Cosmetic changes to spacing, braces, indentation, and location of variable declarations.
Added temp variables to simplify the lengthy equations.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <math.h>
#include <stdint.h>

const double pi_exactly=3.14159265358979323846;

int main(void)
{
    int      m       = 0;
    int      g       = 0;
    uint64_t inbound = 0;

    srand(time(NULL));

    printf("Quantity of random points per square: \n");
    scanf("%d", &m);

    printf("Gridsize: \n");
    scanf("%d", &g);

    double rand_multiplier = 1.0 / ((double) RAND_MAX * g);

    for (int i=0; i<g; i++) {
        double x  = (double) i / g;
        double x1 = (double) (i+1) / g;
        for (int j=0; j<g; ++j) {
            double y  = (double) j / g;
            double y1 = (double) (j+1) / g;

            if (x1*x1 + y1*y1 <= 1) {
                inbound += m;
                continue;
            } else if (x*x + y*y > 1) {
                break;
            }

            for (int k=0; k<m; k++) {
                double xr = x + rand() * rand_multiplier;
                double yr = y + rand() * rand_multiplier;

                if (xr*xr + yr*yr <= 1)
                    inbound++;
            }
        }
    }

    double pi = (double) inbound / ((double) m * g*g)*4;
    double uncertainty = pi - pi_exactly;

    printf("The approximation of pi is: %lf\n", pi);
    printf("Uncertainty: %e\n", uncertainty);
    return 0;
}


Filtering a list for certain strings I want to create a function that filters a list with the following criteria:

Strings containing numbers should be removed
Strings containing only one word and first letter of the word is not uppercase should also be removed.
Strings containing more than one word and first letter of each words except first word is not uppercase should also be removed.

The amount of code I am using for this task seems excessive. Can I produce the same results with something more compact? If yes, how?

def list_filter(str_list):
    filtered = []
    is_end = False
    for string in str_list:
        words = string.split(' ')
        for i, word in enumerate(words):
            if i > 0:
                if word[0].isupper():
                    continue
                else:
                    is_end = True
                    break
            if word.isdigit():
                is_end = True
                break
            if len(words) == 1 and not word[0].isupper():
                is_end = True
                break
        if is_end:
            break
        else:
            filtered.append(string)
    return filtered

I took the liberty of rewriting most of this from scratch for a few reasons.

First, this very difficult to actually test. All your embedded if statements... oh my! Second it's also difficult to add new checks into if you want other filter conditions.

Code like your original code starts out well intentioned but becomes a giant spaghetti mess fairly quickly - you only have three conditions and it's already a complex and hard to read and quickly understand function.

I moved each conditional check into a separate, small function:

# Convention: returns true if should be removed, based on criteria
def contains_numbers(s):
    return any(char.isdigit() for char in s)

Any is a great use for things like this. It will return True if any condition in the list matches.

def single_word_is_not_all_caps(s):
    if len(s.split(' ')) == 1:
        return s.upper() != s
    return False

def contains_multiple_words_not_uppercase(s):
    words = s.split(' ')
    if len(words) > 1:
        return words[0][0].upper() != words[0][0]
    return False

Fairly straightforward here. Both these test specific conditions that can be optimized a bit better in single functions.

This can streamline your main logic significantly (using any again):

 filters = [contains_numbers,
       contains_multiple_words_not_uppercase,
       single_word_is_not_all_caps]

def filter_strings(strs):

    filtered = []
    for s in strs:
        if not any(foo(s) for foo in filters):
            filtered.append(s)

    return filtered

Notice how easy it is now to add new check conditions, you just need to make/test a small function and add it to your filter list. Then iterate over that list, checking if any of them return true for the current string.

You could make this a list comprehension and even simpler if you want:

def filter_strings_list_comp(strs):
    return [s for s in strs if not any(f(s) for f in filters)]

Adding new logical checks is suddenly trivial. Changing existing ones, similarly. With one main "if tree to end all if trees" you will over time end up with a complete mess.

This approach also has the added feature/benefit of making testing much more trivial (I used basic assert statements here):

assert(True == contains_multiple_words_not_uppercase('foo bar'))
assert(False == contains_multiple_words_not_uppercase('Foo bar'))
assert(False == contains_multiple_words_not_uppercase('Foo'))

assert(True == contains_numbers('123'))
assert(True == contains_numbers('1asdf23'))
assert(False == contains_numbers('Foo bar'))
assert(False == contains_numbers(''))

assert(False == single_word_is_not_all_caps('FOO'))
assert(False == single_word_is_not_all_caps('foo bar'))
assert(False == single_word_is_not_all_caps('Foo bar'))
assert(True == single_word_is_not_all_caps('Foo'))

assert(['FOO'] == filter_strings(['Foo', 'FOO']))
assert(['BAR', 'Foo bar'] == filter_strings(['Foo', 'foo', 'BAR', 'Foo bar', '123']))

assert(['FOO'] == filter_strings_list_comp(['Foo', 'FOO']))
assert(['BAR', 'Foo bar'] == filter_strings_list_comp(['Foo', 'foo', 'BAR', 'Foo bar', '123']))

This is a lot easier to isolate problems, particularly if this ends up with many filters.

I would recommend adding more tests here, there are conditions I'm not sure how you want to test or what they should return.


Checking whether a number is a Smith number I have created a Java program to check if the number entered by the user is a Smith number (a number whose sum of digits equals the sum of the digits of its prime factorization). The program works fine but takes too long to check numbers like 4937775. How do I reduce the time taken by it to execute?

import java.util.*;
class check
{
    static boolean a;
    static boolean checkprime(long n)
    {
        int i, factors= 0;
        for(i = 1;i<=n ;i++)
        {
            if(n%i==0)
                factors++;
        }
        if(factors == 2)
            return true;
        else
            return false;
    }

    static long sumfinder(long  n)
    {
        long  sum = 0;
        while(n!=0)
        {
            sum = sum + n%10;
            n= n/10;
        }
        return sum;
    }

    static boolean checksmith(long  n)
    {
        long  sum = 0;
        if(checkprime(n)== true)
        {
            System.out.println(n + " is a prime number ");
            return false;
        }
        else
        {
            System.out.println(n + " is not  a prime number ");
            //generate prime factors:
            long  num = n;

            outer:
            for(long  i = 1; i<= num ; i++)
            {
                if(checkprime(i)== true)
                {

                    if(num%i== 0)
                    {
                        sum= sum+ sumfinder(i);
                        num = num/i;
                        i = 1;
                        if (num == 1)
                        {
                            break outer;
                        }
                    }
                }
            }
        }

        System.out.println(sum);
        System.out.println(sumfinder(n));

        if (sumfinder(n)== sum)

            return true;

        else
            return false;
    }

    static void display()throws InputMismatchException
    {
        Scanner sc = new Scanner(System.in);
        System.out.println("Enter a number");
        long  n = sc.nextInt();
        if(checksmith(n)== true)
        {
            System.out.println("Smith number");

        }
        else
            System.out.println("Not a Smith number");
    }

    public static void main(String [] args)
    {
        display();
    }
}

class check

Java naming convention is that class names should start with an uppercase letter. Check is also not very informative: what does it check?

    static boolean a;

Is this used anywhere?

    static boolean checkprime(long n)

Again, Java naming convention is that (a) camel-case should be used (checkPrime instead of checkprime); (b) Boolean-valued properties should generally have names beginning is (isPrime instead of checkPrime). In this case isPrime reads quite naturally in context (e.g. if (isPrime(n)) ...).

        int i, factors= 0;
        for(i = 1;i<=n ;i++)
        {
            if(n%i==0)
                factors++;
        }
        if(factors == 2)
            return true;
        else
            return false;

Why give i such a wide scope?

Why use factors at all? If you find a divisor which is not 1 or n you can return false immediately.

What's going on with the whitespace in for(i = 1;i<=n ;i++)? The semicolon separates expressions: I can't understand why you'd want whitespace inside the expression i = 1 but not after the semicolon which separates the 1 from i<=n.

There are much faster ways to test primality. Aside from the comments made in another answer about only testing up to sqrt(n), look around for discussion on BPSW or just use BigInteger.isProbablePrime.

    static long sumfinder(long  n)

Again, names. If the name of a method is a noun, I expect it to describe the object returned: but the long returned is not a sum-finder. It's the sum. I suggest that a better name for this method would be digitSum.

    {
        long  sum = 0;
        while(n!=0)
        {
            sum = sum + n%10;
            n= n/10;
        }
        return sum;
    }

I understand this to be at the level of throwaway code rather than production: if it were production code, I'd expect to see either some code to handle the case n < 0 or a comment to explain why that is unnecessary.

    static boolean checksmith(long  n)

See previous comments on names.

Existing answers propose some better ways to write the body of this method; I want to focus as much on general points of style as on algorithms, although I do have one or two points to add there too.

        long  sum = 0;

Which sum? There are two relevant sums, right?

        if(checkprime(n)== true)

== true is a crime against KISS.

            System.out.println(n + " is a prime number ");

That's an unexpected side effect.

            //generate prime factors:

I'm not sure what exactly the scope of this comment is, but I wouldn't describe the following loop as generating prime factors.

            outer:

?? There's no inner loop, so what purpose does this label serve?

            for(long  i = 1; i<= num ; i++)

That should strike terror into the heart of anyone who wants their program to terminate in a reasonable time. There are three possible cases: (a) i could perfectly well be an int; (b) i gets incremented by more than 1 elsewhere; (c) this loop will execute more than \$2^{31}\$ times. Here it's not case (b), so which of the other two is it?

(Aside: the rewrites correctly observe that you only need to go up to sqrt(n), but if n is a long that could still be about \$2^{31.5}\$. It might be worth looking at faster factorisation methods, starting with Pollard's rho).

                if(checkprime(i)== true)
                {
                    if(num%i== 0)

In addition to previous comments, why check whether i is prime? That check is slower than the num % i check, and if the second check succeeds then the first one must also for basic number-theoretic reasons.

                    {
                        sum= sum+ sumfinder(i);

Here you could have an early-abort if the partial digit sum of prime factors already exceeds the digit sum of the original number. That same idea carries over into the suggested rewrites of the method.

        if (sumfinder(n)== sum)

            return true;

        else
            return false;

Those six lines could be simplified to return sumfinder(n) == sum;

    static void display()throws InputMismatchException

Um. If the method is called display, I expect it to do output. Why is it throwing an input-related exception?


Verify credit card number with Luhn's algorithm #include <stdio.h>
    #include <cs50.h>

int main(void){

     long long card;
     int i;
     int otherdigs;
     int multiply;
     int sum = 0;
     int sumofothers = 0;
     int finaladd;
     int length;
     int card_length;
     long long get_card;

     get_card = get_long_long(" Enter credit card: ");
     card = get_card;

     for (i = 0; i < 16; i++)
     {

       sumofothers += card % 10;

       card /= 10;
       otherdigs = card % 10;
       card /= 10;

       if (otherdigs*2 < 10)
        {
         multiply = otherdigs * 2;
         sum += multiply;
         }

       else
         {
          otherdigs = otherdigs*2-9;
          sum += otherdigs;
         }

       }

      finaladd = sum + sumofothers;

      if (finaladd % 10 == 0)
      {
        for (length = 0; length < 14; length++)
        {
            get_card /= 10;
        }

        if ( get_card >= 51 && get_card < 56 ) {

            printf("MasterCart\n");
        }

        else if ( get_card >= 3 && get_card < 4)
        {
            printf("AmericanExpress\n");
        }

        else if ( get_card < 0 || (get_card/10 >= 4 && get_card/10 < 5) )
        {
            printf("VIZA\n");
        }

        else {

            printf(" Sorry, We Don't Support This Payment Company\n");
        }
      }

      else
      {
        printf("This credit card is not valid \n");
      }

      return 0;

}

I recently took cs50 online course and this is my solution to pset1 ( Credit )
This is my very first program that I actually thought through and did it all myself.
The goal is to get the card number from user and check if it's valid ( Luhn’s Algorithm )

I know the style is awful and the code is very messy too, but your opinion is very important to me... I spent maybe 5,6 hours for this. The question is do you think I can code? Become a programmer ? I mean do I have the right mind for it ? ( since this is my first without guide code )
BTW, I have no prior programming experience (only read some pages of K&R 2 years ago),

I even don't know half of c syntax yet. I know talent is a myth and all, but based on this code, do you think I'm any good at this?

the cs50.h that I have does not have a function: get_long_long(), so, for me, the code does not compile.  My compiler suggested: getlonglong()

which credit card company is, normally, contained in the left digits of the card number, not in the right digits. so the posted code logic is (probably) not correct.

The posted code fails to take into account the security code found on the back of the credit card.

Not all credit cards use 16 digits, but rather varies by which vendor is producing the card. So that assumption should not be in the code logic.

Suggest reading the digits as a string rather than as a long long int.  Remember that a character digit - '0' results in a value 0...9, so the conversion, digit by digit is easy.

OT: when compiling, always enable the warnings, then fix those warnings. ( for gcc, at a minimum use: -Wall -Wextra -Wconversion -pedantic -std=gnu11 ) Note other compilers use different options to produce the same results.

the posted code, when run through the compiler, results in the following:

gcc   -O1  -ggdb -Wall -Wextra -Wconversion -pedantic -std=gnu11  -c "untitled1.c"  -I.

untitled1.c: In function ‘main’:

untitled1.c:17:17: warning: implicit declaration of function ‘get_long_long’; did you mean ‘GetLongLong’? [-Wimplicit-function-declaration]
      get_card = get_long_long(" Enter credit card: ");
                 ^~~~~~~~~~~~~
                 GetLongLong

untitled1.c:26:23: warning: conversion to ‘int’ from ‘long long int’ may alter its value [-Wconversion]
        sumofothers += card % 10;
                       ^~~~

untitled1.c:29:20: warning: conversion to ‘int’ from ‘long long int’ may alter its value [-Wconversion]
        otherdigs = card % 10;
                    ^~~~

untitled1.c:14:10: warning: unused variable ‘card_length’ [-Wunused-variable]
      int card_length;
          ^~~~~~~~~~~

Compilation finished successfully.

regarding: Compilation finished successfully  This only means the compiler provided some workaround for each of the problems it found in the code.  It does NOT mean the correct/expected code was produced.

Regarding you question about your suitability to become a programmer.

All programmers started some where, usually fumbling through many many programs before they started thinking of problems in terms of the needed code to solve the problem, so your doing quite well and will get better and better as you gain experience.

regarding:

I even don't know half of c syntax yet.

the C language is not that big.  besides what you have in the posted code, there are pointers and the while() and switch() statement.  Note: learning pointers can be a real pain.  After that there are many many standard C library functions. In Linux, in a 'terminal' window, type man functionName to learn all about a function.  The 'man' command is your best friend, After some 40+ years of programming, I still use that function regularly.


Inserting and deleting elements in a list In my code, I use fast method of reading input and a custom model of list. After reading the input into my 'list'.

The problem is that, when I test my code, most answers are correct, but for some INPUT my algorithm works slow. I unfortunately don't know the input data, but 6 from 12 tests finished with the wrong time. Maybe for some input data, some of the loops do not stop.

Where time is 1.01 or 2.01 -> wrong

  TASK

  Consider any indexed sequence of natural numbers C, which define the
  concept of the present position. Next, we introduce two operations on
  the elements of this sequence: If amount of sequence of natural
  numbers is even number ->

  R, remove the item 'c' on index POS + 1, then move the pointer POS on
  'c' elements to the right, otherways ->

  X, inserting element 'c' on the position (POS + 1) value 'c-1' from
  POS, and then moving the pointer on the POS 'c' elements to the right.

  INPUT:

  the number of times of repetition scheme of operations R and X, the
  sequence of natural numbers 'C'

  OUTPUT:

  numbers after operations from the position of index

  LIMITS

  The time complexity of O (tn), where 'n' is the length of 'C' and 't'
  - times of repetition operations R and X.

#include<iostream>
#include<stdlib.h>
#define gc getchar_unlocked

struct Element
{
    unsigned int c;
    Element* next;
    Element(unsigned int value)
    {
        c = value;
        next = NULL;
    }
};

void scan_integer(unsigned int* o )
{
    unsigned int c = gc();
    int x = 0;
    for( ; ((c<48 || c>57)); c = gc() );

    for( ;c>47 && c<58; c = gc() ) {
        x = (x << 1) + (x << 3) + c - 48;
    }
    *o = x;
}

int main()
{
    unsigned int rounds;
    scan_integer(&rounds);
    if(!feof(stdin))
    {
        unsigned int elements = 0;
        unsigned int tmp;
        scan_integer(&tmp);
        Element* first = new Element(tmp);
        elements++;
        Element* current = first;
        while(!feof(stdin))
        {
            scan_integer(&tmp);
            current->next = new Element(tmp);
            current = current->next;
            elements++;
        }
        current->next = first;
        current = first;
        for(int i = 0; i < rounds; i++)
        {
            if(elements == 0){
                fprintf(stdout, "%u", -1);
                break;
            }
            unsigned int value;
            if(current->c & 1)
            {
                value = current->c;
                Element* newC = new Element(value-1);
                newC->next = current->next;
                current->next = newC;
                elements++;
            }
            else
            {
                value = current->next->c;
                Element* toDelete = current->next;
                current->next = toDelete->next;
                delete toDelete;
                elements--;
            }

            if(elements > 0 && value != 0){
                for(int j = 0; j < value; j++){
                    current = current->next;
                }
            }
        }
        if (elements == 0) {
            fprintf(stdout, "%u", -1);
        }
        if(elements > 0)
        {
            first = current;
            fprintf(stdout, "%u", current->c);
            current = current->next;
            while(first != current)
            {
                fprintf(stdout, " %u", current->c);
                current = current->next;
            }
        }
    }else{
        return -1;
    }

    return 0;
}

Let's start with the code style, even though not asked explicitly.

It says C++, but apart from using a constructor on the struct  and using new/delete, that is pure C what you have written there.

Especially the C style input and output handling is overcomplicated, with regards to how much simpler the C++ interfaces are to use.

As for your implementation, you don't need a linked list. In fact, you don't need to store the input nor the output sequence at all, at least not for the algorithm itself.

Take a look at the problem carefully again, and try to think about how you can rephrase the operations, when do you need to consume input, and what the earliest point is, at which you can create output. Last but not least, how long do you even need to store each specific piece of information.

The first observation:
Whenever an operation has finished, the value at POS is no longer mutated, and no further insertions or deletions occur prior to POS.

The second observation:
During an operation at POS, no value other than POS has an effect on that operation. At most POS+1 is either removed or added in.

The third observation:
As all numbers are guaranteed to be natural numbers, even c - 1 > 0 must hold. Which means that in any case, POS+1 is always output in the same operation as which it is mutated in.

With that in mind, it's actually quite simple to solve this problem:

#include <iostream>
#include <vector>

int main()
{
    std::vector<int> input;
    int rounds = 0;
    bool even = false;

    // Collect the input
    std::cin >> rounds;
    int temp;
    while(std::cin >> temp) {
        input.push_back(temp);
    }

    even = (input.size() % 2) == 0;

    std::vector<int>::iterator it = input.begin();

    // Special handling for first element
    std::cout << *it;
    it++;

    for(int round = 0; round < rounds; round++) {
        // Number of steps to the right
        int steps = *it;
        if(even) {
            // Operation "R"
            // Skip one
            it++;
            // And pass through from input
            for(int i = 0; i < steps; i++, it++) {
                std::cout << " " << *it;
            }
        } else {
            // Operation "X"
            // Directly output `POS+1`
            std::cout <<  " " << *it - 1;
            // But pass through one less input in return
            for(int i = 0; i < steps - 1; i++, it++) {
                std::cout <<  " " << *it;
            }
        }
        even != even;
    }
    for(; it != input.end(); it++) {
        std::cout <<  " " << *it;
    }
}

It is unfortunately still necessary to store the entire input in memory, in order to count the elements. If it wasn't for that, you could just copy from input to output element wise.

But what's important: The data structure used is now a plain vector, which removes a lot of unnecessary stress on the memory system, as the storage method is a compact as it gets.

There are also no mutations to that data structure at all, it only serves to replay the input after counting all elements.