Parametrize by type constructor? I made a tree structure (Tree) that represents an algebraic expression that contains two binary opperators the "question mark" Q/"?" and the "exclamation mark" E/"!".
Both are associative (that means a!(b!c)=(a!b)!c, and the same for the questionmark; this means the order of evaluation doesn't matter and we can omit the parenthesis if we have a chain of the same operator), we do however have no precedence rules, that means we have to use parenthesis if we have a mix of operators: a?b!c would be invalid, it must be (a?b)!c or a?(b!c).
So now I wanted to make it a Show instance that prints out this expression in one line according to the rules outlined above. For that I wrote some helper functions surroundX that add parenthesis where needed.
So my question is: Is there a way to reduce the duplicated code, especially in the surroundX functions?
I thought that it would be nice to be able to pass E or Q as an additional parameter, but I didn't find a way to make that work. But maybe there are other ways to simplify this.

data Tree = Unit | E Tree Tree | Q Tree Tree

surroundE a@(E _ _) = "(" ++ show a ++ ")"
surroundE x = show x
surroundQ a@(Q _ _) = "(" ++ show a ++ ")"
surroundQ x = show x

instance Show Tree where
 show Unit = "1"
 show (E a b) = surroundQ a ++ "!" ++ surroundQ b
 show (Q a b) = surroundE a ++ "?" ++ surroundE b

--example output
main = do
 print $ E (E Unit Unit) (E Unit (Q Unit Unit))
 print $ Q (E Unit Unit) (Q Unit (Q Unit Unit))

Try it online!

Food for thought:
From my (now deleted) comment:
Your operators are associative, but your data structure is not. Is there a different data representation which is associative?
I think you want to do more with this tree than just printing it, right?
The problem is you want Q Unit (Q Unit Unit) to be equal to Q (Q Unit Unit) Unit. Find a 'canonical' representation!
Think about an answer or scroll down to find mine. The next steps just set it up.
Pulling out the operator?
You've already seen this in the comment:
data Operator = ExclamationMark | QuestionMark
data Tree = Unit | BinaryNode Operator Tree Tree

Type parameters
Your example seems to be a minimal working example, but you certainly want your Unit to contain some identifier, e.g. data BinTree a = Leaf a | BinaryNode Operator BinTree BinTree. Do the same with Operator and you've got:
data BinTree op a = Leaf a | BinaryNode op (BinTree op a) (BinTree op a)

Based on your haskell experience, you might want to write your Functor, Applicative, Monad and Traversable instances.
The type parameter op can be instantiated with Operator, but maybe also String in the context of (pretty-)printing. Or go further and have a function in place of op:
evalBinTree :: BinTree (a -> a -> a) a -> a
evalBinTree (Leaf x)                  = x
evalBinTree (BinaryNode f left right) = f (eval left) (eval right)

of course get yourself a function to make these tasks easier:
mapOp :: (op -> op') -> BinTree op a -> BinTree op' a

Associativity Hint
data Binary a = Binary a a is isomorphic to a pair (a,a), which always contains two as. What data structure allows you to have an arbitrary number of as? Next section is some sort of filler to hide the solution.
Haskell philosophy
Haskell is a functional language. While "functional" puts the emphasis on functions, it is actually data-centric. Defining new data structures and converting between them is easier and more concise than in, say, Java. What I am trying to say here: You can keep the BinTree and the AssocTree of the following section and write functions that transforms one representation to the other.
Associativity Solution
First a non-parametric data type that allows an arbitrary number of children:
data AssocTree = Unit | Node [AssocTree]

but I will continue with:
data AssocTree op a = ALeaf a | ANode op [AssocTree op a]

And left as an exercise, you want a function:
toAssocTree :: BinTree op a -> AssocTree op a

But toAssocTree cannot collect your associative operators. That needs equality on op:
collectAssoc :: (Eq op) => AssocTree op a -> AssocTree op a

This way, you divide your computation into little steps.
Why turn Operator into a type parameter op
Here is the reason why I supposed to make the operation a type parameter: With an abstract type, the type system prevents this error:
data BTree = Unit | E Tree Tree | Q Tree Tree
data ATree = AUnit | AE [Tree]  | AQ [Tree]
toATree (E left right) = AE [left,right]
toATree (Q left right) = AE [left,right] -- c&p error: should be AQ, not AE

Yes, abstracting out the type prevents errors.


Pass class property as parameter to extension method I have the following class structure:

MyAttribute.cs

public sealed class MyAttribute : Attribute
{
    public MyAttribute(string name)
    {
        if (string.IsNullOrEmpty(name))
        {
            throw new ArgumentNullException(nameof(name));
        }

        Name = name;
    }

    public string Name { get; }
}

Matrix.cs

public class Matrix
{
    [MyAttribute("awesome_name")]
    public string AwesomeName { get; set; }
}

And we make an method to "extract" the value defined on name of MyAttribute:

AttributeExtensions.cs

public static class AttributeExtensions
{
    public static string GetMyAttributeNameOfProperty<TClass>(Expression<Func<TClass, object>> expression)
    {
        if (expression.Body is MemberExpression memberExpression)
        {
            var propertyInfo = (PropertyInfo)memberExpression.Member;

            var hasMyAttribute = propertyInfo.IsDefined(typeof(MyAttribute), false);

            if (hasMyAttribute)
            {
                return propertyInfo.GetCustomAttribute<MyAttribute>().Name;
            }
        }

        return default;
    }
}

Finally, we call this way:

AttributeExtensions.GetMyAttributeNameOfProperty<Matrix>(exp => exp.AwesomeName)

This call, returns: "awesome_name"

We have some doubts:

Exists an more "clean" way to pass the property of <TClass>(exp.AwesomeName) to this extension method?
We really need of Expression<>? It's possible to make this only with Func<TClass, object>?

Like:

GetMyAttributeNameOfProperty<TClass>(Func<TClass, object> expression)

Any suggestions?

I'd suggest:

public static class AttributeExtensions
{
    public static string GetMyAttributeNameOfProperty<TClass>(string propertyName)
    {
        var propertyInfo = typeof(TClass).GetProperty(propertyName);
        return propertyInfo?.GetCustomAttribute<MyAttribute>()?.Name ?? default;
    }
}

Usage:

var awesome = AttributeExtensions.GetMyAttributeNameOfProperty<Matrix>(nameof(Matrix.AwesomeName));
Console.WriteLine(awesome);

Note:  The above isn't an extension method.  The below code is.

public static class AttributeExtensions
{
    public static string GetMyAttributeNameOfProperty<TType>(this TType me, string propertyName) where TType : Type
    {
        var propertyInfo = me.GetProperty(propertyName);
        return propertyInfo?.GetCustomAttribute<MyAttribute>()?.Name ?? default;
    }
}

Usage:

var awesome = typeof(Matrix).GetMyAttributeNameOfProperty(nameof(Matrix.AwesomeName));
Console.WriteLine(awesome);

Both output:

  awesome_name

Benefits:

Does not use an Expression or even the namespace
System.Linq.Expressions
Won't throw an exception (returns null if property or attribute doesn't exist)
Using nameof makes refactoring safe in the event you rename class properties
Does not use Func<TClass, object> or similar
Does not require an instance of Matrix (or any other type since it's generic)

Either way works and is really a matter of preference.  There are pros and cons to using extension methods.


Hailstones and the Collatz Conjecture The Collatz Conjecture is related to a type of problem called a Hailstone problem; because hailstones circulate up and down through a cloud around a planet until they eventually fall. The Collatz conjecture posits that for the function

$$
f(n) = \begin{cases} \text{even: } & n/2\\ \text{odd: } & 3n+1\end{cases}
$$

the sequence \$n, f(n), f(f(n)), \cdots, f^i(n), \cdots\$ will eventually arrive at the value 1.

The following example code, which calculates the sum of all even members of the sequence obtained for a given starting value \$n\$, is given as part of an exercise, but I'm looking to improve it. By improving it, I mean: shorten via composition; via mapping; via folding; or any other more powerful and better practice.

foo :: Integer -> Integer
foo 1 = 0
foo n
  | even n    = n + foo (n `div` 2)
  | otherwise = foo (3 * n + 1)

Here's what I got. I'm using point-free style with composition  in foo'. However, I'm not so sure how to improve f.

foo' :: Integer -> Integer
foo' = (sum . f)

f :: Integer -> [Integer]
f x
 | x == 1    = []
 | even x    = x : f (x `div` 2)
 | otherwise = f (3*x + 1)

Please critique this improvement.

Every piece of code should have a clear purpose. If you need to conduct some maintenance on a piece of code, you obtain its description first. You consult users, manuals, previous developers as needed.

I assume this code is bug-free and it's doing some calculation on some hailstone numbers.

First of all, you extracting the implicit sequence is an improvement on the original, provided we know we are dealing with a sequence.

Further improvements

That only even numbers are included in the sum is not part of the Collatz Problem. By moving the condition we can rename the unnamed sequence as hailstones. Also f is not a good name for a sequence, as f and g usually denote parameters which are functions.

foo' :: Integer -> Integer
foo' = sum . filter even . hailstones -- removed unnecessary parentheses

hailstones :: Integer -> [Integer]
hailstones x
 | x == 1    = []
 | even x    = x : hailstones (x `div` 2)
 | otherwise = x : hailstones (3 * x + 1) -- space around *

Stopping condition is not part of the original Collatz problem, either. Hailstone sequences do not stop at 1, but rather go  \$ \cdots, 1, 4, 2, 1, 4, 2, \cdots \$ forever.

foo' :: Integer -> Integer
foo' = sum . filter even . takeWhile (/= 1) . hailstones

hailstones :: Integer -> [Integer]
hailstones x
 | even x    = x : hailstones (x `div` 2)
 | otherwise = x : hailstones (3 * x + 1)

Make the recursive nature of these sequences explicit:

collatzStep n
 | even n    = n `div` 2
 | otherwise = 3 * n + 1

hailstones x = x : collatzStep x

Last line can be written as hailstones = iterate collatzStep.

Altogether:

collatzStep n
 | even n    = n `div` 2
 | otherwise = 3 * n + 1

hailstones = iterate collatzStep

foo' = sum . filter even . takeWhile (/= 1) . hailstones

Now, of course if you were to hand this in as a homework, TAs or the professor would have asked you to add a lot of comments. First of all you do not tell them that "Comments are a code smell." Or describe what you did as KISS, DRY, readability etc.

Since this probably is a Programming Languages homework, you should add a comment mentioning that collatzStep is "lambda lifted", not "factored out"; that hailstones is an anamorphism and this, of course, makes foo an hylomorphism (correct the greek, and add more of it as necessary desired).


Aggregate summary counts for an array of error objects I'm doing some refactoring of someone else's code, and just want a second opinion, because of course I think my work makes it better, but some validation (or correction) would be helpful.

Starting with an array like this:

errorLog: [{
 errorCode: 11,
 errorDescription: "abc",
 date: "2017-01-01",
 severity: "H"
},{
 errorCode: 11,
 errorDescription: "abcd",
 date: "2017-01-02",
 severity: "H"
},{
 errorCode: 99,
 errorDescription: "abcd",
 date: "2017-01-02",
 severity: "H"
}]

and trying to get results like this:

errorSummary: [{
 errorCode: 11,
 severity: "H",
 count: 2
},{
 errorCode: 99,
 severity: "H",
 count: 1
}]

this is the existing code:

//instead of this, which is hard to reason about and debug (and includes a line that will never rturn true: if (hardErrorsSorted.includes...)):
let hardErrors = testData.filter(ts1 => ts1.severity === 'H');
let hardErrorsSorted = hardErrors.sort(this.mySorter);
for (let i = 0; i < hardErrorsSorted.length; i++) {
  if (i != hardErrorsSorted.length - 1) {
    if (hardErrorsSorted[i].errorCode != hardErrorsSorted[i + 1].errorCode) {
      let errorCount = this.getCount(hardErrorsSorted, hardErrorsSorted[i].errorCode);
      this.errorDataList.push({
        errorCode: hardErrorsSorted[i].errorCode,
        errorCodeType: 'H',
        errorCodeTotalCount: errorCount
      });
    }
  } else {
    if (hardErrorsSorted.includes(hardErrorsSorted[i].errorCode, 0)) {
    } else {
      let errorCount = this.getCount(hardErrorsSorted, hardErrorsSorted[i].errorCode);
      this.errorDataList.push({
        errorCode: hardErrorsSorted[i].errorCode,
        errorCodeType: 'H',
        errorCodeTotalCount: errorCount
      });
    }
  }
}

and my refactoring:

//use something like this, which is much easier to grasp at a glance, doesn't jump around, and is DRYer
let hardErrorCodes = testData.filter(ts => ts.severity === 'H').map(v => v.errorCode);
let hardErrorCounts = {};

//sum up the unique errors
for (let error of hardErrorCodes) {
  if (!(error in hardErrorCounts)) {
    hardErrorCounts[error] = 0;
  }
  hardErrorCounts[error]++;
}

//add the summed error counts to the master list
for (let error in hardErrorCounts) {
  this.errorDataList.push({
    errorCode: error,
    errorCodeType: "H",
    errorCodeTotalCount: hardErrorCounts[error]
  });

What do you all think? Is this a helpful refactor, or a waste of time?

Here’s another one towards functional using es6 syntax additions:

const predSeverity = severity => err => err.severity == severity
const byCode = (acc, {errorCode: code}) =>
  acc.set(code, (acc.get(code) || 0) + 1)

errorLog.filter(predSeverity("H")).reduce(byCode, new Map())

It has a bit different result data format (and type, if only for the receiver-as-return-value of Map::set), since the filtering already tells us the severity.


JQuery Tabs in C# MVC to stay on reload I am working on a C# MVC website. in which i am using tabs.I was trying to stay on the current tab when page refreshes.

I achieved this by following code:

$(document).ready(function () {
    if (window.location.href.indexOf("?") > -1) {
        var newURL = location.href;
        window.history.pushState('object', document.title, newURL);
    }
});
$(document).ready(function () {
    $('a[data-toggle="tab"]').on('show.bs.tab', function (e) {
        localStorage.setItem('activeTab', $(e.target).attr('href'));
    });
    var activeTab = localStorage.getItem('activeTab');
    if (activeTab) {
        $('#myTab a[href="' + activeTab + '"]').tab('show');
    }
});

This is working fine. But if the page is load by RedirectToAction from another page then it showing last opened tab, I needed to show the first tab.

So I added a hidden input field in the html.

<input type="hidden" value="@Model.IsRefresh" id="IsRefresh" />

On $(document).ready i change url as follows

$(document).ready(function () {
    if (window.location.href.indexOf("?") > -1) {
        var newURL = location.href.replace("?str=true", "");
        window.history.pushState('object', document.title, newURL);
    }
});

and in GET checks for this bool str and set IsRefresh as false

#region Check Is Page Refresh
if (str != null)
{
    if ((bool)str)
    {
        item.IsRefresh = false;
    }
    else
    {
        item.IsRefresh = true;
    }
}
else
{
    item.IsRefresh = true;
}
str = false;
#endregion

so if IsRefresh is false then it is page refresh and else it is RedirectToAction.

So i changed the code as:

$(document).ready(function () {
    $('a[data-toggle="tab"]').on('show.bs.tab', function (e) {
        localStorage.setItem('activeTab', $(e.target).attr('href'));
    });
    var activeTab = localStorage.getItem('activeTab');

    <!-- added these lines -->
    var refresgh = $("#IsRefresh").val();
    if (!refresgh) {
        activeTab = "";
    }

    if (activeTab) {
        $('#myTab a[href="' + activeTab + '"]').tab('show');
    }
});

Now its working fine, but sometimes i get the feeling that on page refresh different tab is showing. Is something wrong with my code?

Is there any better solution??

If browser compatibility matrix satisfies your requirements (see CanIUse) then there is a simpler way to check if page is reloaded, read window.performance.navigation.type value. For reload event value is TYPE_RELOAD then you may simply drop your hidden input and:

if (window.performance.navigation.type != 1) {
    activeTab = "";
}

See also W3C specs for further details.

If you're not targeting IE you may want to use template literals instead of string concatenation, for example:

$(`#myTab a[href="${activeTab}"]`).tab('show');

Value you're persisting in localStorage does not need to be retained across different sessions then you'd better use sessionStorage instead. It also has the advantage to be isolated by window (or tab).

You may want to use let and, where appropriate, const instead of var.

To put everything all together you may want to revert the condition you're checking to simplify your code:

$(document).ready(function () {
    $('a[data-toggle="tab"]').on('show.bs.tab', function (e) {
        sessionStorage.setItem('activeTab', $(e.target).attr('href'));
    });

    if (window.performance.navigation.type === 1) {
        const activeTab = sessionStorage.getItem('activeTab');
        $(`#myTab a[href="${activeTab}"]`).tab('show');
    }
});

The recommended way to attach an handler to the ready event is:

$(function () {
    // ...
});

Absolutely nothing wrong with the verbose way (which BTW I find more clear).

I add this only because you may find it useful in future, to detect a page refresh may be done also server side, see asp.net mvc - Detecting page refresh.


Classifying a branch as master or dev I am dealing constantly with the following code.

The thing is that literally it makes no sense for me to have all the spacing and divisions in the if-else when the code is so small that can be a block by itself.

When I read a line within line breaks I expect something difficult to understand or something that should be paid special care to be read about. Not just a line break after each function or if definition by default.

if (branchInfo.name === BRANCHNAMES.MASTER) {

    branchInfo.isMaster = true;

} else if (
    branchInfo.name === BRANCHNAMES.RELEASE ||
    branchInfo.name === BRANCHNAMES.HOTFIX
) {

    branchInfo.isDev = true;

}

Given that the line doesn't exceed the 120 characters and that they are small truthy statements that don't deserve to get a function, I want to remove that unnecessary split in the else if because it doesn't seem necessary, almost like the other line breaks and in my opinion it just leads into weird code.

if (branchInfo.name === BRANCHNAMES.MASTER) {
    branchInfo.isMaster = true;
} else if (branchInfo.name === BRANCHNAMES.RELEASE || branchInfo.name === BRANCHNAMES.HOTFIX) {
    branchInfo.isDev = true;
}

Am I being over paranoid about it? Is this actually a good practice?

PS: I know a good practice is to keep into a coding style and always keep it as it is, but really, I can't stand some things that just make code look weird. Sometimes too many line breaks also lead into unreadable code, don't you think so?

Personally, I see no problem doing it either way. I prefer to keep my lines shorter than that but 120ish characters isn't unreasonable.

Another option you have is to store those values in some other variables and use those instead.

let isMaster = branchInfo.name === BRANCHNAMES.MASTER;
let isRelease = branchInfo.name === BRANCHNAMES.RELEASE;
let isHotfix = branchInfo.name === BRANCHNAMES.HOTFIX;
if (isMaster) {
    branchInfo.isMaster = true;
} else if (isRelease || isHotfix) {
    branchInfo.isDev = true;
}

Another way would be to use a switch statement with case fallthrough. Some people refuse to allow fallthrough so it's your call.

switch (branchInfo.name) {
    case BRANCHNAMES.MASTER:
        branchInfo.isMaster = true;
        break;

    case BRANCHNAMES.RELEASE:
    case BRANCHNAMES.HOTFIX:
        branchInfo.isDev = true;
}

Finally, I would consider just assigning directly.

branchInfo.isMaster = branchInfo.name === BRANCHNAMES.MASTER;
branchInfo.isDev = branchInfo.name === BRANCHNAMES.RELEASE ||
                   branchInfo.name === BRANCHNAMES.HOTFIX;

There's also one trick I use if I need to compare against multiple things at once.

let devBranches = [BRANCHNAMES.RELEASE, BRANCHNAMES.HOTFIX];
branchInfo.isMaster = branchInfo.name === BRANCHNAMES.MASTER;
branchInfo.isDev = devBranches.includes(branchInfo.name);

If you don't perform any extra work in either branch of the if statement, I would use the last or second to last option. If you need to perform additional work, I would use the additional variables to make it more readable.


Assign a value to combobox depend upon other combobox value I have a windows form in C# project that keeps some information. I created a bunch of textbox and combobox dynamically, depend upon user input.

Consider, I have a list of 10 combobox whose values will be selected by user.

The combobox values are:

Finding
No Finding
Incomplete
Skipped.

Now I have a single final combobox  which contains the following values.

Finding
No Finding
Incomplete
Skipped Reviewed
Skipped Not reviewed

The final combobox  should be populated depend upon the following logic

If all values are "Finding",  then the the final combobox should be "Finding"
If all values are "No_Finding", then the the final combobox should be "No_Finding"
If all values are "InComplete", then the the final combobox should be "InComplete"
If all values are "Skipped", then the the final combobox should be "Skipped Not Reviewed".
If any value is "Finding", then the final combobox should be "Finding".
If any value is "Incomplete", then the final combobox should be "Incomplete".
If any value is "Skipped", then the final combobox should be "Skipped Reviewed".

With respect to the above logic conditions I have written the following that works fine.

After writing the code I feel it's not cleaner and not an easily understandable solution.

What I need now whether there is any way to refactor the following lines of code.

 public void selectfinalComboxValue()
        {
            List<string> list_of_combobox = new List<string>();
//txtBoxValLines is an user input
            for (int i = 0; i < int.Parse(txtboxvalLines.Text); i++)
            {
                string cmbboxValue = ((ComboBox)panel1.Controls["Add_combobox" + (i).ToString()]).Text;
                list_of_combobox.Add(cmbboxValue);

            }
            List<string> distinct = list_of_combobox.Distinct().ToList();

            if (distinct.Any(str => str.Contains("InComplete")))
            {
                cmbFinalStatus.SelectedIndex = 2;
                cmbFinalStatus.Enabled = false;
            }
            else if (distinct.Any(str => str.Equals("Finding")))
            {
                cmbFinalStatus.SelectedIndex = 0;
                cmbFinalStatus.Enabled = false;
            }
            else
            {
                cmbFinalStatus.SelectedIndex = -1;
                cmbFinalStatus.Enabled = true;
            }

            if (distinct.Count().ToString() == "1")
            {

                if (distinct.Any(str => str.Equals("Skipped")))
                {
                    cmbFinalStatus.SelectedIndex = 4;
                    cmbFinalStatus.Enabled = false;
                }
                else if (distinct.Any(str => str.Equals("No Finding")))
                {
                    cmbFinalStatus.SelectedIndex = 1;
                    cmbFinalStatus.Enabled = false;
                }
            }
            else {
                if (distinct.Any(str => str.Contains("Skipped")))
                {
                    cmbFinalStatus.SelectedIndex = 3;
                    cmbFinalStatus.Enabled = false;
                }

            }
        }

I think you might be able to eliminate creating the distinct list and the nested ifs by first checking for the All() condtions, then later check for Any()

    var items = list_of_combobox.ToList();

    if (items.All(str => str.Equals("Finding")))
    {
       // assign final comboxbox
    }
    else  if (items.Any(str => str.Equals("Finding")))

Also, you might want to try creating an enum of the possible values, adding these values to the ComboBox, then getting/setting the SelectedItem property rather than dealing with indexes.


Get n-th perfect number The task:

  A number is considered perfect if its digits sum up to exactly 10.

  Given a positive integer n, return the n-th perfect number.

  For example, given 1, you should return 19. Given 2, you should return
  28.

My solution:

const sum = (acc, m) => acc + Number(m);
const getNthPerfectNumber = n => {
  let i = 0, x = 0;
  while (i !== n) {
    x++;
    if (x
      .toString()
      .split('')
      .reduce(sum, 0) === 10) { i++; }
  }
  return x;
};

console.log(getNthPerfectNumber(2));

Computers do numbers!

Compared to manipulating numbers, strings are very slow and use a lot of memory relative to what is being done.

To test if it is a perfect integer

const isPerfectInt = num => {
    var sum = (num | 0) % 10;
    while (num > 0) { sum += (num = num / 10 | 0) % 10 }
    return sum === 10;
}

Improved cache

You are caching results in your answer and you only cache what you are looking for. However while you are searching you find all the perfect values below the one you are looking for. This negates most the advantage you get from caching calculations.

If you keep all calculated perfect numbers you can then use that last calculated value as a starting point. That way you will never test the same value twice.

const cache = [0,19];  // set up first value. Saves some messing about
const getNthPerfectNumber = n => {
    if (cache[n]) { return cache[n] }

    // Start from the last number tested plus one.
    var idx = cache.length - 1, num = cache[idx] + 1;
    while (idx < n) {
        if (isPerfectInt(num)) { cache[++idx] = num }
        num++;
    }
    return num - 1;
};


Read file into list when class is instantiated in Python 2.7 I would like to read a file into a list when I create the class below. Once the list is ready, I would like to access it outside the class in order to process more code based on the list which now contains my file. Can the code be improved upon?

class ReadFile(object):
    def __init__(self, filename):
        self._data = []
        self.localdir = 'C:\\Users\\User\\Documents'
        self.filename = filename
        self.filepath = os.path.join(self.localdir, self.filename)
        self.read_data()

    def read_data(self):
        try:
            with open(self.filepath, 'rU') as c:
                rows = csv.reader(c)
                for row in rows:
                    self._data.append(row)
        except Exception as ex:
            print(ex)

    def get_data(self):
        return self._data

def main():
    new = ReadFile('myfile.csv')
    new_data = new.get_data()

    for i in new_data:
        print(i)        # etc... more code will follow

if __name__ == '__main__':
    main()

I agree with Caridorc about not needing a class, in addition ReadFile isn't a very good class name, it sounds like a function. Unless, you meant read as an adjective (this file has been read), but you can see how that could be confusing.

Keeping filename and your hardcoded path seems redundant at the moment

    self.localdir = 'C:\\Users\\User\\Documents'
    self.filename = filename
    self.filepath = os.path.join(self.localdir, self.filename)

Also you could use r''to write your filepath without the double backslash. r before a string will tell Python to interpret it literally, ie. Don't consider anything an escape character. That means that a backslash will always just be a backslash.

You store all 3 of these but only every use filepath, why not go directly to creating the filepath? The point of init should be taking the passed parameters and turning them into the form needed for your class, and your class only needs filepath. Particularly if you do convert this to a function, it'll become less useful to keep the redundant values.

Also, since init is about getting the data you need, why even have getData as a separate function? Collapse that back into init since it's being called there anyway and will never be called again. In fact if you call get_data() twice you've accidentally doubled up on the info, which you surely never want.

Since you're not changing the data at all, you can pass the csv reader directly to list() to create a list. In fact, you could just return list(rows) if you turned this all into one function.

You should add a docstring to your class/function. ReadFile seems pretty clear until you find out that you've passed a filepath and gotten an error that it doesn't exist (since you read from a specific directory) or you might be puzzled when you pass a text file and get back a csv formatted file.

Here's how I might refactor your code:

def read_csv_file(filename):
    """Returns a list of data from a csv file passed to it.

    Only reads files from 'C:\Users\User\Documents\' + filename
    Prints any exceptions from reading the file."""

    filepath = os.path.join(r'C:\Users\User\Documents', filename)

    try:
        with open(filepath, 'rU') as c:
            rows = csv.reader(c)
            return list(rows)
    except Exception as ex:
        print(ex)

def main():
    new_data = read_csv_file('myfile.csv')
    for i in new_data:
        print(i)        # etc... more code will follow

if __name__ == '__main__':
    main()

If you were sticking with keeping it a class then I'd still keep the above in one function and make minor other changes. I'd add a docstring about the class's general use.

Also I would no longer catch and print the error. Errors shouldn't be ignored, and failing to read your file will actually mean that the class hasn't been correctly created as it's missing crucial data and likely has an invalid path. You can either let it be normally raised without intervention, or add a custom error message with raise ErrorType("Message about the error"). In your case IOError is likely, so you might raise IOError with a message about how the passed filename isn't in your hardcoded directory. But exactly how you'll handle that is up to you.

Like this:

class ReadFile(object):
    """Docstring"""

    def __init__(self, filename):
        self.localdir = r'C:\Users\User\Documents'
        self.filename = filename
        self.filepath = os.path.join(self.localdir, self.filename)

        try:
            with open(self.filepath, 'rU') as c:
                rows = csv.reader(c)
                self._data = list(rows)
        except IOError:
            raise IOError("{} not found in {}".format(
                          self.filename, self.directory)
    def get_data(self):
        return self._data


In a list of pairs, replace snds with median of all snds with the same fst Given is a list of pairs. I want to replace the second element of all pairs with the median of the second elements of all pairs with the same first element.

For example:

[("a",1.0),("b",3.0),("a",2.0),("a",4.0)]
->
[("a",2.0),("b",3.0),("a",2.0),("a",2.0)]

The order of the elements in the resulting list is not important. So the following result would be equally valid:

[("a",2.0),("a",2.0),("a",2.0),("b",3.0)]

My current solution looks as follows:

import Control.Arrow
import qualified Data.Map as M

median :: Fractional a => [a] -> a
median xs | null xs  = error "empty list"
          | odd  len = xs !! mid
          | even len = meanMedian
                where  len = length xs
                       mid = len `div` 2
                       meanMedian = (xs !! mid + xs !! (mid+1)) / 2

solve :: [(String, Float)] -> [(String, Float)]
solve pairs = map replaceWithMedian pairs
    where
        medianMap = map (mapSnd (:[])) >>> M.fromListWith (++) >>> M.toList
                        >>> map (mapSnd median) >>> M.fromList
        mapSnd f (x, y) = (x, f y)
        replaceWithMedian (x, y) = (x, M.findWithDefault 0 x (medianMap pairs))

main :: IO ()
main = print $ solve [("a",1.0),("b",3.0),("a",2.0),("a",4.0)]

solve feels unnecessary long to me. I'm looking for more elegant/terse solution, not character-count-wise but with a simpler algorithm.

Fixing things first

First of all, median is wrong:

median [1,2,3,4,5] /= median [3,1,4,5,2]

Both should return 3, but on the first list median returns 3 and on the latter 4. Make sure to sort your list, or state in the (currently missing) documentation that your list has to be sorted.
Also, using !! twice is rather slow. Instead, drop the first elements up to mid and then take the mean:

median :: (Fractional a, Ord a) => [a] -> a
median [] = error "median: empty list"
median xs
 | odd len  = xs' !! mid
 | otherwise = mean $ take 2 $ drop mid $ xs'
 where
     len = length xs
     mid = len `div` 2
     xs' = sort xs

mean :: (Fractional a) => [a] -> Maybe a
mean xs = -- exercise; it's not trivial to get this right

-- Alternatively, just use meanDuo in median:
meanDuo :: Fractional a => [a] -> a
meanDuo [x,y] = -- ...

Also, if you use error, make sure to include the function's name in the error message; best practice include the completely qualified name, i.e.

error "Data.TupleMagic.median: empty list"

After all, there are (usually) no stack traces, and it's really nice to know where things went wrong. Note that you should add this behaviour in a documentation, since the type doesn't really tell what would happen on an empty list. Since Fractional a is also an instance of Num, any fixed number could be feasible. Alternatively use Maybe a as return type or NonEmpty as input.

Use the tools you have at hand

mapSnd is second from Control.Arrow. Also, Data.Map is an instance of Functor, which means you can replace

M.toList >>> map (second f) >>> M.fromList

with

fmap f

Furthermore, be honest. Although medianMap is called *Map, it's actually a function. While GHC might call it only once, it's not guaranteed, so remember the map. And last, since you know that every key x will be contained in toMap pairs, you can use Data.Map.! instead of Data.Map.findWithDefault:

solve :: [(String, Float)] -> [(String, Float)]
solve pairs = map replaceWithMedian pairs
  where
    toMap     = map (second pure) >>> M.fromListWith (++) >>> fmap median
    medianMap = toMap pairs
    replaceWithMedian (x, _) = (x, medianMap M.! x)

Furthermore, there's usually no reason to use Float nowadays, unless you've checked

that it's faster,
that it's reduced accuracy fulfils your needs, and
you're having memory constraints (which usually also contradicts String and lists).

Instead, Double is used most of the time.


Factorising and Finding Prime numbers with list comprehension I have created a simple code which finds the factors of any given integer. It then compares the value of the input to the range of numbers. Because a prime number can factorize into 1 and itself. Then I search length of the factor list. If the list contains 2 or less elements it is added to the all_primes list. I have flattened the list and also produced a list with the composite numbers missing for pattern recognition.

Is this a good method to find the Prime numbers and factors of a value and is there any bugs I can encounter?

all_primes = []

def user_in(num):
    while True:
        if num <= 2:
          break
        num -= 1
        numbers = [i for i in range(1, num + 1)]
        factors = [i for i in numbers if num % i == 0]
        primes = [i for i in factors if len(factors) <= 2 and i != 1]
        all_primes.append(primes)
        print('Number: {}\nFactors: {}\nFactor count: {}\nSum of factor: {}\nSum of numbers: {}\nSum of Primes: {}\n\n'.format(num, factors, len(factors), sum(factors), sum(numbers), sum(primes), primes))

x = int(input('Enter number: '))
user_in(x)
all_primes.reverse()
flat_list = [i for k in all_primes for i in k]
print('Flat list of Primes: {}\n\nSum of all primes: {}\n\nAll Primes: {}'.format(flat_list, sum(flat_list) + x, all_primes))

This example is part of the output:

Number: 5
Factors: [1, 5]
Factor count: 2
Sum of factor: 6
Sum of numbers: 15
Sum of Primes: 5

Number: 4
Factors: [1, 2, 4]
Factor count: 3
Sum of factor: 7
Sum of numbers: 10
Sum of Primes: 0

Number: 3
Factors: [1, 3]
Factor count: 2
Sum of factor: 4
Sum of numbers: 6
Sum of Primes: 3

Number: 2
Factors: [1, 2]
Factor count: 2
Sum of factor: 3
Sum of numbers: 3
Sum of Primes: 2

Flat list of Primes: [2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47]

Sum of all primes: 378

All Primes: [[2], [3], [], [5], [], [7], [], [], [], [11], [], [13], [], [], [], [17], [], [19], [], [], [], [23], [], [], [], [], [], [29],[], [31], [], [], [], [], [], [37], [], [], [], [41], [], [43], [], [], [], [47], [], []]

I have to state right at the start, I don't know what your code does.  That's ok.  I'm going to stream-of-conciousness review the code, so you'll get all my thoughts and improvements as it goes on.  Maybe that's a good thing.

Global Variables

Global variables are frowned upon.  They make code harder to test and harder to reason about.  The function user_in modifies all_primes.  What value(s) can all_primes have before the function runs?  If the function is run more than once, does more stuff get added to all_primes?  Does stuff get replaced?  Is the result corrupted?

In this case, the global variable is easy to get rid of.  Instead of modifying a global all_primes variable, how about returning it?

def user_in(num):
    all_primes = []
    while True:
        ...
        all_primes.append(primes)
        ...
    return all_primes

...
all_primes = user_in(x)
...

Now you can run user_in() several times with different input values, and actually test that the right value is returned in your test cases.

Method Naming

What does user_in() do?  It sounds like it might ask the user for some input, but it isn't clear what that input might be.

But reading the user_in() method, it doesn't ask for any input at all!  Rather, the mainline code ask the user to enter a number, and this number is given as an input to the user_in() method.  A better name would be process_and_output(), but that is still a horrible name; it doesn't say what kind of processing is being done, nor what kind of output is produced.  Moreover, the method is producing output and it is (now) returning a value.  Functions should do one logical thing, and be named after what it does.

Ranges, Lists, and Iterables

The statement numbers = [i for i in range(1, num + 1)] constructs a range object with the numbers from 1 to num inclusive, takes successive numbers in that range, and extracts them into a list.  The remainder of the code iterates over the values stored in numbers.

Why?

First of all, numbers = list(range(1, num + 1)) would be a shorter way of expressing the construction of numbers.

Second, why realize a physical list of numbers when you are just going to iterator over it, and a range() object itself is iterable?  Just use the range object.

numbers = range(1, num + 1)

Factorization

factors = [i for i in numbers if num % i == 0]

This is fine, but it is inefficient.  Factors come in pairs.  If num % i == 0, then i is a factor and num // i is also a factor.  If you recorded both, then instead of looping over all numbers, you could loop up to math.isqrt(num) + 1.  If num is one million, this would save you nine hundred and ninety nine thousand trial divisions!  Note, you would have to watch for the special cases of num being a perfect square: you would want to generate that factor exactly once.

Move Constant Expressions Out of Loops

What does this statement do?

    primes = [i for i in factors if len(factors) <= 2 and i != 1]

First, look at the len(factors) <= 2 condition.  Can it change during the loop, or will it always be either True or False?  The list of factors doesn't change, so clearly the length is constant.  So we could write this as a two statements:

    if len(factors) <= 2:
        primes = [i for i in factors if True and i != 1]
    else:
        primes = [i for i in factors if False and i != 1]

Simplifying if True and anything to if anything, as well as if False and anything to "never", we get:

    if len(factors) <= 2:
        primes = [i for i in factors if i != 1]
    else:
        primes = []

Moreover, if there are 2 or less factors, those factors are 1 and num.  Since you are filtering out the i == 1 factor, you will always be left with the second factor, if it exists, which will always be num:

    if len(factors) == 2:
        primes = [num]
    else:
        primes = []

Then, you are adding this list to all_primes, and later flattening the list, to remove the empty lists which you added.

How about, just adding the prime you found to all_primes, and not generating a list of lists?

    if len(factors) == 2:
        all_primes.append(num)

Printing

That is a rather long print statement.  Any reason for making it that long?  Maybe split it up into the equivalent 7 print statements?  We've gotten rid of primes, which contained 0 or 1 value, so computing the "sum of primes" needed to be adjusted.  It begs the question as to what the "sum of primes" was supposed to be?  Why does it produce 0 if the number wasn't prime?

    print('Number: {}'.format(num))
    print('Factors: {}'.format(factors))
    print('Factor count: {}'.format(len(factors)))
    print('Sum of factor: {}'.format(sum(factors)))
    print('Sum of numbers: {}'.format(sum(numbers)))
    print('Sum of Primes: {}'.format(num if len(factors) == 2 else 0))
    print(''.format(primes))

Wait a second ... what is that last print statement printing?  ''.format(primes)?  Did you have an argument to the format statement that didn't have a corresponding {} code???

Python 3.6+ now has something called an f-string, which allows you to interpolate values and expressions directly into the string, so you will never mismatch .format(*args) again!

    print(f'Number: {num}')
    print(f'Factors: {factors}')
    print(f'Factor count: {len(factors)}')
    print(f'Sum of factor: {sum(factors)}')
    print(f'Sum of numbers: {sum(numbers)}')
    print(f'Sum of Primes: {num if len(factors) == 2 else 0}')
    print()

Looping a Fixed Number of Times

def user_in(num):
    while True:
        if num <= 2:
            break
        num -= 1
        ...

This code wants to loop starting from num - 1, and go down and stop at 2.  This is what for variable in range(): statements were meant for.  Don't recreate for loops by using while True loops and modifying the loop index yourself:

def user_in(start):
    for num in range(start - 1, 2, -1):
        ...

Sum of all primes

print('... Sum of all primes: {} ...'.format(..., sum(flat_list) + x, ...))

What is that + x doing there?  If I give as input the number 4, it computes the primes less than 4 to be [2, 3], and the sum to be 9!  I'm pretty sure this is wrong.

Reworked Code

I've reworked the code a little bit, trying to reason out what you were actually trying to do.  For example "Sum of primes" I implemented a running total of the prime numbers being generated.  I also implemented the faster factorization method, I mentioned above, as a separate function.  For good measure, I added a sum_up_to function for computing \$\sum_{i=1}^{n} i\$.  Finally, the main function has been renamed to find_primes_up_to(), since that seems to be what it is doing, and the mainline code has been moved into a if __name__ == '__main__': guard, so the file can be imported into other files, and allows unit tests to be added.

import math

def factorize(num):
    if num == 1:
        return [1]

    limit = math.isqrt(num) + 1

    small = [1]
    large = [num]
    for i in range(2, limit):
        if num % i == 0:
            small.append(i)
            large.append(num // i)

    if small[-1] == large[-1]:
        large.pop()

    return small + large[::-1]

def sum_up_to(num):
    return sum(range(1, num + 1))

def find_primes_up_to(limit):
    sum_of_primes = 0
    primes = []

    for num in range(1, limit + 1):
        factors = factorize(num)

        print(f'Number: {num}')
        print(f'Factors: {factors}')
        print(f'Factor count: {len(factors)}')
        print(f'Sum 1 to {num}: {sum_up_to(num)}')

        if len(factors) == 2:
            primes.append(num)
            sum_of_primes += num
            print(f'Sum of primes: {sum_of_primes}')

        print()

    return primes

if __name__ == '__main__':
    limit = int(input("Enter upper limit: "))
    primes = find_primes_up_to(limit)
    print(f'Primes: {primes}')
    print(f'Sum of all primes: {sum(primes)}')


Simplify IActionResult return value logic It would be nice to simplify the verbosity of this:

public IActionResult Get(int id)
{
    var movie = context.GetMovie(id);

    if (movie != null)
    {
        return Ok(movie);
    }
    else
    {
        return NotFound();
    }
}

with something like this:

public IActionResult Get(int id)
{
    var movie = context.GetMovie(id);
    return (movie != null) ? Ok(movie) : NotFound();
}

Of course, this is an error because the two objects in the ternary must be of the same type, not just sharing the same inheritance (or is this incorrect?).

The closest I can get is to explicitly cast one of the values as an IActionResult:

return (movie != null) ? Ok(movie) : (IActionResult) NotFound();

Is there a better option?

So, as mentioned in comments: the code is readable and there are no issues with "verbosity". And as mentioned you can get rid of else statement, also it's a good habit to exclude "bad" cases at first.

public IActionResult Get(int id)
{
    var movie = context.GetMovie(id);

    if (movie == null)
    {
        return NotFound();
    }

    return Ok(movie);
}


Canonical Implementation of Move Semantics I am trying to compose an illustrative example which shows how to implement move semantics on an object that will be stored in a vector.

Please consider the following code, which is my illustrative example so far. It is designed to be a canonical, pedantically correct implementation of an object that implements move semantics, does not implement copy semantics, and can be stored in a vector<> (Where T is Moveable, below). How did I do?

#include <string>
#include <iostream>
#include <vector>
#include <list>
#include <algorithm>
#include <memory>
#include <iterator>
using namespace std;

class Moveable
{
public:
    string foo_;
    string bar_;

    Moveable(Moveable&& rhs) : foo_(std::move(rhs.foo_)), bar_(std::move(rhs.bar_)) {}  // move construction
    Moveable(const string& foo) : foo_(foo) {};             // convert construction
    Moveable& operator=(Moveable&& rhs)     // move assignment
    {
        foo_ = std::move(rhs.foo_);
        bar_ = std::move(rhs.bar_);
        return * this;
    }
private:
    Moveable(const Moveable&);              // not defined, not copy-constructible
    Moveable& operator=(const Moveable&);   // not defined, not copy-assignable
    Moveable();                             // not defined, not default constructible
};

Moveable generate_it()
{
    static string foo ;
    if( foo.empty() || foo[0] == 'z' )
        foo.insert(0, 1, 'a');
    else
        foo[0]++;
    return foo;
}

int main()
{
    typedef vector<Moveable> Moveables;
    Moveables v;
    generate_n(back_inserter(v), 1024, &generate_it);
    cout << v.size() << endl;
    string target = "zzz";
    auto that = find_if(v.begin(), v.end(), [target](const Moveables::value_type& it) -> bool
    {
        return it.foo_ == target;
    });

    if( that != v.end() )
        v.erase(that);

    sort(v.begin(), v.end(), [](const Moveables::value_type& lhs, const Moveables::value_type& rhs) -> bool
    {
        return lhs.foo_ > rhs.foo_;
    });
    cout << v.size() << endl;
}

One small error: Mark the move constructor as noexcept, otherwise there are situations where it won’t be used. The linked case is different since the class has a copy-constructor, yet I can imagine that there are still situations where it matters, especially since your copycon isn’t deleted, just private and undefined.
Why is the destructor virtual? If the example is supposed to be minimal then this might be distracting.
In generate_it:

return std::move(Moveable(foo));

The std::move here is redundant, since you are returning a temporary. What’s more, the explicit constructor call is redundant too, since the constructor isn’t marked as explicit. Just return foo; will do.
The find_if call could be replaced by vanilla find, using temporary construction again:

auto that = find(v.begin(), v.end(), target);

Finally, in the sort call, why aren’t the arguments declared const&? Granted, makes the line even longer as it stands this is inconsistent with the const-correctness illustrated in the find_if call.


AWS Lambda Python function, to enable / disable access to a VPC environment We have a requirement that access to an AWS environment is enabled on demand, and said access disabled afterwards. In each case (up / down), this means we will do two things:

Enable access

Turn on the Bastion instance
Modify the Security Groups of the EC2 instances, so a SG allowing RDP access is added to the list of associated SGs.

Disable access

Turn off the Bastion
Modify the SGs of the EC2 instances, to remove the SG that grants RDP access.

The script is pretty straight forwards, but there is not much error handling / logging going on.

The script is designed to run either from local machine or in AWS lambda, without modification (providing environment variables, credentials/permissions setup correctly).

import boto3, os

region = os.environ['PFregion']
security_group_id = os.environ['PFSG']
bastion_iid = os.environ['PFbastion_iid']
aws_profile = 'profile_name'

def lambda_handler(event, context):
    if (__name__ == "__main__"):
        print 'using ENV creds'
        session = boto3.Session(profile_name=aws_profile)
        ec2_client = session.client('ec2', region_name=region)
        ec2_resx = session.resource('ec2', region_name=region)
    else:
        ec2_client = boto3.client('ec2', region_name=region)
        ec2_resx = boto3.resource('ec2', region_name=region)

    all_instances = ec2_resx.instances.filter()

    if (event['up'].lower() == 'true'):
    # Start the Bastion
        print 'inside up'
        ec2_client.start_instances(
            InstanceIds = [bastion_iid,],
            AdditionalInfo = 'Called from Lambda function to Up access',
            #DryRun = event['dry_run'],
        )
        # Modify the SGs accordingly
        for instance in all_instances:
            all_sg_ids = [sg['GroupId'] for sg in instance.security_groups]
            if security_group_id not in all_sg_ids:
                print 'SG not assigned currently'
                all_sg_ids.append(security_group_id)
                instance.modify_attribute(Groups=all_sg_ids)

    else:
        # Stop the Bastion
        print 'inside down'
        ec2_client.stop_instances(
            InstanceIds = [bastion_iid,],
            #DryRun = bool(event['dry_run']),
        )
        # Remove the SG rules
        for instance in all_instances:
            all_sg_ids = [sg['GroupId'] for sg in instance.security_groups]
            if security_group_id in all_sg_ids:
                print 'SG IS assigned currently'
                all_sg_ids.remove(security_group_id)
                instance.modify_attribute(Groups=all_sg_ids)

if __name__ == '__main__':
    event = { "up" : "true", "desc" : "this is a description field", "dry_run" : "false" }
    #print event
    lambda_handler(event, None)

A recommendation that I give with most of my reviews, and which is relevant here, is a perusal of PEP 8, the official style guide for Python. Many of my comments below will reference that document.

import boto3, os

PEP 8 has a few things to say about this (in the section entitled Imports). First, generally you shouldn't put multiple modules on the same line. Next, third-party modules should be distinguished from built-ins, preferably with a blank line (and local application modules after). In addition, I like to alphabetize my imports within each group. It can prevent duplicates. So I would do it like this:

import os

import boto3

region = os.environ['PFregion']
security_group_id = os.environ['PFSG']
bastion_iid = os.environ['PFbastion_iid']
aws_profile = 'profile_name'

I have several problems with this. For one thing, os has a shortcut function for accessing environment variables: os.getenv. I would also pass these as arguments to lambda_handler instead of using global variables. This gives the option of calling the function from another module or calling it multiple times without the need of setting environment variables.

The if __name__ == '__main__': block should be where those variables are gotten. As a nice touch, you could use argparse to add commandline arguments as an alternative to environment variables. I would probably do it something like this:

# At the top with your other imports
from argparse import ArgumentParser

# In `if __name == '__main__':`
parser = ArgumentParser(description="Your program description")
parser.add_argument('-r', '--region', description='PF region')
 # (Repeat for other arguments)

args = parser.parse_args()

Then you can pass something like args.region or os.getenv('PFregion') to your function. That gives the command line option as the decision-maker, but it falls back on the environment variable.

def lambda_handler(event, context):
    if (__name__ == "__main__"):

This is a worker function; it shouldn't care what is calling it. I would move the block outside the function and pass the relevant variables as arguments. You could even use something like:

# Though because of my previous recommendation, this function would have more arguments.
def lambda_handler(event, context, session=boto3):
    ec2_client = session.client('ec2', region_name=region)
    ec2_resx = session.resource('ec2', region_name=region)

Then your if __name__ == '__main__': block could simply pass the session as an argument, but the default without a session still works.

event = { "up" : "true", "desc" : "this is a description field", "dry_run" : "false" }

I would add these options to the environment variable and command line options.

Python has its own booleans. Use them: "up": True.

Another styling comment: I wouldn't put spaces on both sides of the braces. PEP 8 comments on this.

I also noticed that you never use event['desc'] or event['dry_run']. Why then are they ever defined?


Church Numerals - implement one, two, and addition Given the following exercise:

  Exercise 2.6

  In case representing
  pairs as procedures wasn't
  mind-boggling enough, consider that,
  in a language that can manipulate
  procedures, we can get by without
  numbers (at least insofar as
  nonnegative integers are concerned) by
  implementing 0 and the operation of
  adding 1 as

(define zero (lambda (f) (lambda (x)
x)))

(define (add-1 n)   (lambda (f)
(lambda (x) (f ((n f) x)))))

  This representation is known as Church
  numerals, after its inventor, Alonzo
  Church, the logician who invented the
  calculus.

  Define one and two directly (not in
  terms of zero and add-1). (Hint: Use
  substitution to evaluate (add-1
  zero)). Give a direct definition of
  the addition procedure + (not in terms
  of repeated application of add-1).

I wrote this solution.  I'm not 100% certain what the correct answer is, and I'm not sure the best way to check my solution so it's possible that I have made a mistake in my answer.  Please let me know what you think.

;given definitions
(define zero (lambda (f) (lambda (x) x)))
(define (add-1 n)
  (lambda (f) (lambda (x) (f ((n f) x)))))
; exercise 2.6: define one and two directly -
; not in terms of zero or add-1
(define one
  (lambda (f) (lambda (x) (f (f x)))))
(define two
  (lambda (f) (lambda (x) (f ((f (f x)) x)))))

(define (church-plus a b)
  (define (church-num n)
    (cond ((= 0 a) (lambda (x) x))
          ((= 1 a) (lambda (f) (lambda (x) (f ((n f) x)))))
          (else (church-num (- n 1)))))
  (church-num (+ a b)))

Church defined numerals as the repeated application of a function.  The first few numerals are defined thus:

(define zero (lambda (f) (lambda (x) x)))
(define one (lambda (f) (lambda (x) (f x))))
(define two (lambda (f) (lambda (x) (f (f x)))))

... and so on.

To see how one is derived, begin by defining one as (add-1 zero) and then perform the applications in order as shown in the steps below (I have written each function to be applied and its single argument on separate lines for clarity):

;; Givens
(define add-1 (lambda (n) (lambda (f) (lambda (x) (f ((n f) x))))))
(define zero (lambda (f) (lambda (x) x)))

(define one
  (add-1    ;; substitute with definition of add-1
   zero))

(define one
  ((lambda (n)    ;; apply this function to argument zero
     (lambda (f) (lambda (x) (f ((n f) x)))))
   zero))

(define one
  (lambda (f) (lambda (x) (f ((zero    ;; substitute with definition of zero
                               f) x)))))

(define one
  (lambda (f) (lambda (x) (f (((lambda (f)    ;; apply this function to argument f
                                 (lambda (x) x))
                               f)
                              x)))))

(define one
  (lambda (f) (lambda (x) (f ((lambda (x)    ;; apply this function to argument x
                                x)
                              x)))))

(define one
  (lambda (f) (lambda (x) (f x))))    ;; we're done!

You may try your hand at two in a similar fashion. =)

By definition, Church numerals are the repeated application of a function.  If this function were the integer increment function and the initial argument were 0, then we could generate all natural numbers 0, 1, 2, 3, ....  This is an important insight.

Thus, a Church numeral is a function that takes one argument, the increment function, and returns a function that also takes one argument, the additive identity.  Thus, if we were to apply a Church numeral to the integer increment function (lambda (n) (+ 1 n)) (or simply add1 in Scheme), which we then apply to the integer additive identity 0, we would get an integer equivalent to the Church numeral as a result.  In code:

(define (church-numeral->int cn)
  ((cn add1) 0))

A few tests:

> (church-numeral->int zero)
0
> (church-numeral->int one)
1
> (church-numeral->int two)
2
> (church-numeral->int (add-1 two))
3
> (church-numeral->int (add-1 (add-1 two)))
4

A Church-numeral addition should take two Church numerals as input, and not integers, as in your code.

We use the insight above about increment functions and additive identities to notice that integer addition is simply repeated incrementing.  If we wish to add integers a and b, we begin with the number b and increment it a times to get a + b.

The same applies to Church numerals.  Instead of using integer increment function and integer additive identity, we use the Church increment function add-1 and we begin with a Church numeral as the additive identity.  Thus, we can implement Church-numeral addition as:

(define (plus cn-a cn-b)
  ((cn-a add-1) cn-b))

A few examples:

(define three (plus one two))
(define four (plus two two))
(define five (plus two three))
(define eight (plus three five))

... and associated tests:

> (church-numeral->int three)
3
> (church-numeral->int four)
4
> (church-numeral->int five)
5
> (church-numeral->int eight)
8


Intersection of the interval set Recently I had an interview where it was asked:

  Given a list of people with their birth and end years find the year with the most number of people alive.

I implemented in Java and was trying to find the optimal solution. I am sure there might be some data structure which will be optimal to use in this scenario.

static class Person {
    int born;
    int died;

    Person(int born, int died) {
        this.born = born;
        this.died = died;
    }
}

/*
 * (1920, 1939),
 * (1911, 1944),
 * (1920, 1955),
 * (1938, 1939),
 * (1920, 1939),
 * (1911, 1944),
 * (1920, 1955),
 * (1938, 1939),
 * (1937, 1940)
 *
 */

public static void main(String[] args) {
    List<Person> people = new ArrayList<>();
    people.add(new Person(1933, 1989));
    people.add(new Person(1920, 1939));
    people.add(new Person(1911, 1944));
    people.add(new Person(1920, 1955));
    people.add(new Person(1938, 1939));
    people.add(new Person(1920, 1939));
    people.add(new Person(1911, 1944));
    people.add(new Person(1920, 1955));
    people.add(new Person(1938, 1939));
    people.add(new Person(1937, 1940));
    System.out.println(solution1(people));

}

public static int solution1(List<Person> people) {
    HashMap<Integer, Integer> peopleMap = new HashMap<>();
    int maxCount = 0;
    int year = 0;
    for (Person p : people) {
        for (int i = p.born; i <= p.died; i++) {
            Integer value = peopleMap.get(i);
            if (value == null) {
                peopleMap.put(i, 1);
            } else {
                value++;
                if (maxCount < value) {
                    maxCount = value;
                    year = i;
                }
                peopleMap.put(i, value);
            }
        }
    }
    return year;
}

HashMap is overkill here.

Depending on the constraints of the problem (mainly the minimum and maximum allowable years), you could simply use an array instead of a HashMap.

Sure HashMaps have a O(1) complexity, but nothing is going to beat just indexing an array.

Better algorithm

Coming up with a better algorithm is very tricky.

Your solution is O(NL) where N is the number of people and L is the average lifetime. But keep in mind that L is practically a constant, so it does not really matter that much.

You could most certainly come up with a O(NlogN) solution, based on creating a sorted list of relevant dates. This would outperform your current solution for small values of N, but better performance at the low end is rarely useful (it can be, but that's the exception, not the rule).

Edit Thinking about this, I suspect that's the "trick" part of that interview question. Overlapping intervals is a classic interview problem. However, making the intervals happen on a fully discretized space makes the hashmap solution viable, whereas it can't be used when dealing with "traditional" float-bound intervals.


Ten thousand and first prime checker takes a very long time I am attempting problem 7 on Project Euler.  I have come up with this solution which works fine for finding smaller nth prime numbers, but really lags when it comes to higher nth prime numbers.  I am not quite sure where to start to make it more efficient.

public class TenThousandFirstPrime {

    public static boolean isPrime(int num) {
        if(num % 2 == 0) return false;
        for(int i = 3; i < Math.floor(Math.sqrt(num)); i += 2) {
            if(num % i == 0) return false;
        }

        return true;
    }

    public static void main(String[] args) {
        int count = 2;
        int i = 3;
        while(count <= 10001) {
            if(isPrime(i)) {
                i += 2;
                count++;
            }
        }
        System.out.println(i);
    }
}

The solution only took a couple of minutes and I didn't expect it to be as slow as it is...

The square root calculation could just be done before the loop.  Since this value does not change within the function, it doesn't need to be recalculated each time through the loop.

public static boolean isPrime(int num) {
    if (num % 2 == 0) return false;

    int squareRoot = Math.floor(Math.sqrt(num));
    for (int i = 3; i < squareRoot; i += 2) {
        if (num % i == 0) return false;
    }

    return true;
}


Quick Sort Implementation with iterators Most quicksort examples online use C-style arrays and indexes. I have written below quicksort implementation with STL style iterators. It looks fine to me, but I feel like I may be making more than necessary copies of iterators. I am open for any suggestions/improvements:

#include <iostream>
#include <vector>
#include <cassert>
#include <algorithm>
#include <iterator>

template <typename Iter>
inline Iter partition(const Iter& beg, const Iter& end) {
  using std::swap;
  assert(beg != end);
  auto piv = end;
  --piv; // Pivot is the last element
  auto index_small = beg;

  for (auto index_large = beg; index_large != piv; ++index_large) {
    if (*index_large <= *piv) {
      swap(*index_large, *index_small);
      ++index_small;
    }
  }
  swap(*index_small, *piv);
  return index_small;
}

template <typename Iter>
void quick_sort(const Iter& beg, const Iter& end) {
  if (end - beg > 1) {
    const auto& piv = partition(beg, end);
    quick_sort(beg, piv);
    quick_sort(piv, end);
  }
}

// Test
int main() {
  std::vector<int> vec{ 19, 54, 53, 9, 3, 96, 40, 36, 96, 43, 45,
                        78, 57, 6, 12, 100, 36, 15, 27, 45, 80, 39,
                        10, 71, 100, 15, 41, 43, 20, 32, 61, 2, 55,
                        33, 85, 32, 11, 50, 25, 17, 75, 74, 39, 49,
                        50, 17, 18, 55, 63, 81, };
  quick_sort(vec.begin(), vec.end());
  std::copy(vec.begin(), vec.end(), std::ostream_iterator<int>(std::cout, " "));
  std::cout << '\n';
}

You have done a great job.  Here’s my suggestions:

Sort the include directives in alphabetical order.
Instead of using the generic name Iter, it may be a good idea to express the random access requirement: something like RandomIt.
Iterators should be passed by value, not const reference.  Don’t be afraid to copy iterators — they are intended to be lightweight objects that are frequently copied, and passing them by const reference will incur overhead over passing them by value because of the additional indirection.
It is more logical to use auto piv = std::prev(end);, because int y = x - 1; is more logical then int y = x; --y;, right?  (Of course you can use end - 1, but that may be less efficient if the subtraction operator of the iterator isn’t inlined.)
You can use std::iter_swap instead of manually ADL’ing swap.
Instead of enforcing <=, consider taking a custom comparator as the standard algorithms do.  (Also note that the standard library facilities always use < as the comparator by default.  <= is not required unless you provide a custom comparator.)

Also, always choosing last element as the pivot can make the code vulnerable to reversely sorted sequences.  You can consider taking a random number generator to randomize the choice if this becomes a problem.


Printing a multiplication table in C I'm a complete beginner, but I'd like to learn how to program. I really started 2 weeks ago.

So I tried the common exercise, aka the multiplication table, but I tried to add little things I learnt recently in order to challenge myself, like functions, tables and pointers even if it's useless here.

The code works well, but I have some warnings when compiling, and I don't know why, like *pointSize = NULL;

Besides, I'm sure my code could be cleaned up, so I'd like you to tell me how I can improve it.

Here's the result:

How long this table should be ? (0 - 50) 5

       |    1  |    2  |    3  |    4  |    5  |
================================================
    1  I    1  |    2  |    3  |    4  |    5  |
-------+-------+-------+-------+-------+-------+
    2  I    2  |    4  |    6  |    8  |   10  |
-------+-------+-------+-------+-------+-------+
    3  I    3  |    6  |    9  |   12  |   15  |
-------+-------+-------+-------+-------+-------+
    4  I    4  |    8  |   12  |   16  |   20  |
-------+-------+-------+-------+-------+-------+
    5  I    5  |   10  |   15  |   20  |   25  |
-------+-------+-------+-------+-------+-------+

/* Multiplication table */

#include <stdio.h>
short tableSize; // Number of multipliers
short question (short *pointSize); // Ask user how many multipliers
void error(short nbUser); // Cherck error
void multTable(); // The table

void main ()
{
    short i = 1, nbMultipliers;
    question(&nbMultipliers);
    error(nbMultipliers);
    tableSize = nbMultipliers;

    /* header */
    printf("       |");
    for (i = 1; i <= tableSize; i++)
    {
        printf("  %3u  |", i);
    }
    printf("\n");

    for (i = 1; i <= (tableSize + 1); i++)
    {
        printf ("========");
    }
    printf("\n");
   /* end header */

    multTable(); // the table
}

/* Ask user how many multipliers */
short question (short *pointSize)
{
    *pointSize = NULL;

    printf("How long this table should be ? (0 - 50) ");
    scanf("%u", pointSize);
    printf("\n");
}

void error (short nbUser)
{
    if (nbUser < 0 || nbUser > 50)
    {
        printf("Error : you must enter a number between 0 and 50");
    }
}

/* end Ask user how many multipliers */

void multTable()
{
    short i = 1, j = 1;
    short line[50] = {0};

    for (j = 1; j <= tableSize; j++)
    {
        printf("  %3u  I", j);
        for (i = 1; i <= tableSize; i++)
        {
            line[i] = i*j;
            printf ("  %3u  |", line[i]);
        }
        printf("\n");
        for (i = 1; i <= (tableSize + 1); i++)
        {
            printf ("-------+");
        }
        printf("\n");
    }

}

You should generally try to avoid global variables (like tableSize). Since they can be used everywhere ("globally"), at the worst case you need to read all of the code to figure out where they come from and how they're used. In this case, tableSize could become a parameter of multTable.

While it will probably work on the compilers you'll come across, void main() isn't strictly guaranteed to work in all C implementations. The standard signature is int main(int argc, char** argv), or possibly int main(void).

Defining multiple variables in a single line, like short i = 1, nbMultipliers;, is generally frowned upon because (a) it can be hard to read, and (b) C made some questionable decisions about the need to repeat qualifiers like const and * on types, making the syntax dangerous.

However, you shouldn't really be defining all of your variables at the top of your functions (this was required in old versions of C, but hasn't been for twenty years). You should define your variables as late as possible.

The signature of question could be simplified to short question(void) -- don't use pointers when you don't have a reason to.

You are calling error incorrectly. You've defined it to take a short, but you're passing it a pointer to a short. It should instead be called like error(nbMultiplier);. The C compiler should tell you this -- are you compiling using warnings? The flags -Wall -Wextra are very useful!

Initializing pointSize prior to reading in is not necessary, and actually possibly harmful to the compiler/a debugger catching your mistakes. Also, NULL is a value for pointers. When you're setting *pointSize, you're setting the value of a short, not a pointer. You probably mean 0 or -1 or some other value - but again, it's simpler to just leave it uninitialized in this case, so that a sanitizer or debugger can catch any accidental uses of uninitialized data.

The format specifier for short is %hd, not %u. Using %u will invoke undefined behavior. Here too, the compiler will emit a warning telling you to fix this.

exit is included in stdlib.h, which you have not #included. It also requires a integer parameter, the exit code to use. Since this is an error, you should use EXIT_FAILURE.

Here is what your program looks like (skipping the body of multTable which we haven't gotten to yet) if you apply all of the above:

#include <stdio.h>
#include <stdlib.h>

short question(void);    // Ask user how many multipliers
void error(short nbUser); // Cherck error
void multTable(short);  // The table

int main(void)
{
    short tableSize = question();
    error(tableSize);

    /* header */
    printf("       |");
    for (short i = 1; i <= tableSize; i++)
    {
        printf("  %3u  |", i);
    }
    printf("\n");

    for (short i = 1; i <= (tableSize + 1); i++)
    {
        printf("========");
    }
    printf("\n");
    /* end header */

    multTable(tableSize); // the table
    return 0;
}

/* Ask user how many multipliers */
short question(void)
{
    short pointSize;
    printf("How long this table should be ? (0 - 50) ");
    scanf("%hd", &pointSize);
    printf("\n");
    return pointSize;
}

void error(short nbUser)
{
    if (nbUser < 0 || nbUser > 50)
    {
        printf("Error : you must enter a number between 0 and 50");
        exit(EXIT_FAILURE);
    }
}

void multTable(short tableSize)
{
......
}

Notice how related question() and error(); you use error() only to evaluate the output of question(), and question()'s output cannot be trusted without invoking error(). Instead of making question() possibly return a bad value, you should make it encapsulate the error detection itself:

short question(void)
{
    short pointSize;
    printf("How long this table should be ? (0 - 50) ");
    scanf("%hd", &pointSize);
    printf("\n");
    if (pointSize < 0 || pointSize > 50)
    {
        printf("Error : you must enter a number between 0 and 50");
        exit(EXIT_FAILURE);
    }
    return pointSize;
}

Also, notice that you have two places that have printfs: both main and multTable. It's good practice to keep each function doing a single understandable thing. main is doing too much in also printing the table header; especially since that table header needs to be changed if you ever change multTable's output format. So, you should move printing the table header into multTable.

In multTable, look carefully at how you're using the line variable. You're only using it to write to line[i], and then immediately read back line[i] -- you're not using it as an array at all! It can simply be replaced with a local short variable.

The resulting multTable will look something like this:

void multTable(short tableSize)
{
    /* header */
    printf("       |");
    for (short i = 1; i <= tableSize; i++)
    {
        printf("  %3u  |", i);
    }
    printf("\n");

    for (short i = 1; i <= (tableSize + 1); i++)
    {
        printf("========");
    }
    printf("\n");
    /* end header */

    for (short j = 1; j <= tableSize; j++)
    {
        printf("  %3u  I", j);
        for (short i = 1; i <= tableSize; i++)
        {
            short product = i * j;
            printf("  %3hd  |", product);
        }
        printf("\n");
        for (short i = 1; i <= (tableSize + 1); i++)
        {
            printf("-------+");
        }
        printf("\n");
    }
}

General comments

First, you should put more care into how you name things.

You should generally avoid abbreviations (unless they are extremely standardized abbreviations). You read all the time, & u dnt gnly rd txtlkths, you read text that looks like this. Don't say nb, say number. Don't say mult, say multiplication.

Functions should generally be named like commands to do something. For example, printMultiplicationTable(). This lets you read code as a sequence of commands, each one saying what it does. Look at how clear and simple the following looks:

int main(void) {
    short tableSize = getTableSize();
    printMultiplicationTable(tableSize);
    return 0;
}

Comments should not state things that are obvious. A comment of // the table adds absolutely nothing to a line that consists of multTable();. Also, you generally don't need /* end .. */ comments. Your text editor or IDE can let you quickly jump to the beginning and end of functions, and "collapse" them. Keeping track of these manually in the code adds very little value but quickly adds a lot of clutter.

You also hardly need to use shorts. They are often dangerously small (real world numbers very quickly get larger than 16,000), and are no longer faster to operate on in modern computers. If you have a very large array of small numbers, then it might be time to use a short instead of an int (though an explicitly sized type, like int16_t, would be even better!)

Your multiplication table accepts input up to 50, but it won't actually work properly for inputs that large. That's because the largest numbers in the resulting table are more than 3 digits wide. Your code should use 4 columns, or you should dynamically adjust the number of columns depending on what's needed for the user's input.

Also, it bears repeating, make sure you compile with compiler warnings on, and address all of the warnings. C is not easy to get right, and C compilers generally give very helpful warnings that catch lots of mistakes that both beginners and veterans make. You should probably use both -Wall and -Wextra if you're using clang or gcc to get all of the warnings turned on.


Reading keyboard input This is how I read the keyboard in my game:

    @Override
public void keyPressed(KeyEvent key) {
    keypressed=true;

    if (key.getKeyCode() == KeyEvent.VK_UP) {
        keyup=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_DOWN) {
        keydown=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_ENTER) {
        keyenter=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_ESCAPE) {
        keyesc=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_W) {
        keyw=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_S) {
        keys=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_A) {
        keya=true;
    }

    if (key.getKeyCode() == KeyEvent.VK_D) {
        keyd=true;
    }
}

Is there a more efficient, simpler, or neater way to do this?

Yes there is. As KeyEvent is an enumeration type, you can instead use a switch-statement:

char key;
boolean isSpecialKey = false;
SpecialKey specialKeyValue;
switch(key.getKeyCode()){
    case KeyEvent.VK_UP:
       key = '';
       isSpecialkey = true;
       specialKeyValue = SpecialKey.ARROW_UP;
       break;
    case KeyEvent.VK_W:
       key = 'w';
       break;
    //Continue...
}

I took the liberty of creating a new SpecialKey enum that is supposed to handle special keys when pressed.  For these, there is also the isSpecialKey flag. You simply wouldn't create a case for Keys that you don't handle. Instead, do nothing in the default case.


Speed-Distance-Time Calculator in Rust 1.8 Whenever I try a new language I always try to write a speed-distance-time calculator as well as possible. I did this with C++ a while ago now. This is a similar question but this time in Rust.

The program calculates speed, distance or time from the other two variables in the equation and the idea is to try to do this without a load of nested if statements and repetition. I try to make as elegant a solution as possible but the program needs to show the working for the equation.

I want to know how I could adjust the program to:

Fit in with good (Rust) practices
Shorten the program as much as possible
Catch all errors
Add support for units and conversions later on as well as other common equations
Make better use of Rust

    use std::io;

// Simple input function to simplify calc functions

fn get_param(s: &str) -> f64 {
    println!("{}", s); // Print input instructions

    loop { // loop until a float is entered
        println!("Enter a floating point number!");
        let mut input = String::new();

        io::stdin().read_line(&mut input)
                .expect("failed to read line");

        let input: f64 = match input.trim().parse() {
            Ok(num) => num,
            Err(_) => continue,
        };

        return input;
    }
}

fn calc_speed() -> f64 {
    let distance: f64   = get_param("Enter distance:");
    let time: f64       = get_param("Enter time:");
    println!("Speed = distance / time");
    println!("Speed = {} / {}", distance, time);
        distance / time
}

fn calc_time() -> f64 {
    let speed: f64      = get_param("Enter speed:");
    let distance: f64   = get_param("Enter distance:");
    println!("Time = distance / speed");
    println!("Time = {} / {}", distance, speed);
        distance / speed
}

fn calc_distance() -> f64 {
    let speed: f64      = get_param("Enter speed:");
    let time: f64       = get_param("Enter time:");
    println!("Distance = speed * time");
    println!("Distance = {} * {}", speed, time);
        speed * time
}

fn main() {
    let mut calc_input = String::new();

    println!("Hello, would you like to calculate speed, distance or time?");

    io::stdin().read_line(&mut calc_input)
            .expect("failed to read line");

    let answer: f64;

    match calc_input.trim() {
        "speed" | "Speed"       => {
            answer = calc_speed();
            println!("Speed = {}", answer);
        }
        "distance" | "Distance" => {
            answer = calc_distance();
            println!("Distance = {}", answer);
        }
        "time" | "Time"         => {
            answer = calc_time();
            println!("Time = {}", answer);
        }
        _                       => {
            println!("Enter speed, distance or time!")
        }
    }

}

I am new to Rust and know only a little about the language, coming primarily from C++. I know if I was writing C++ I would probably write a sdt class and maybe a struct for working.

You have extra indentation on the use statement and in your implicit returns.
I don't like the detail printed between the prompt and the entry of the value:

Enter distance:
Enter a floating point number!

I'd reorder these two lines, or combine them.
There's no need for the match in the prompt to evaluate to a value, it can directly return from the function.
This change allows you to switch to an if let statement because you'd only care about one variant.
If you keep the loop structure to read strings, you could consider keeping one allocated String to store into. You need to be careful to reset the string between loops, but it's a bit more efficient.
There's no need to specify the type on variables in the calc_* functions. Type is defined by the call to get_param.
Could extract some duplication for the prompt "Enter ...:" and move it to the prompt function.
Actually, "prompt" is a better verb than "get"; "get" is almost always a weak verb.
Don't declare a variable with a wider scope than needed - declare answer inside the match arms in main.
Actually, there's no reason to have a variable for answer at all, inline the call into the println!.
Actually, why split the printing inside and outside of the calc_* functions? Might as well do it all in one place.
What about supporting various cases of input like "SpEED"? Could match against a lowercased version of the string.

use std::io;

fn prompt_for_value(value_name: &str) -> f64 {
    println!("Enter {}:", value_name);

    loop {
        println!("Enter a floating point number!");

        let mut input = String::new();
        io::stdin().read_line(&mut input).expect("failed to read line");

        if let Ok(num) = input.trim().parse() {
            return num;
        }
    }
}

fn calc_speed() {
    let distance = prompt_for_value("distance");
    let time     = prompt_for_value("time");
    println!("Speed = distance / time");
    println!("Speed = {} / {}", distance, time);
    println!("Speed = {}", distance / time);
}

fn calc_time() {
    let speed    = prompt_for_value("speed");
    let distance = prompt_for_value("distance");
    println!("Time = distance / speed");
    println!("Time = {} / {}", distance, speed);
    println!("Time = {}", distance / speed);
}

fn calc_distance() {
    let speed = prompt_for_value("speed");
    let time  = prompt_for_value("time");
    println!("Distance = speed * time");
    println!("Distance = {} * {}", speed, time);
    println!("Distance = {}", speed * time);
}

fn main() {
    println!("Hello, would you like to calculate speed, distance or time?");

    let mut calc_input = String::new();
    io::stdin().read_line(&mut calc_input).expect("failed to read line");

    match calc_input.to_lowercase().trim() {
        "speed"    => calc_speed(),
        "distance" => calc_distance(),
        "time"     => calc_time(),
        _          => println!("Enter speed, distance or time!"),
    }
}

Here's an alternate version of prompt_for_value that I might have written, but it's a bit different.

use std::io::BufRead;

fn prompt_for_value(value_name: &str) -> f64 {
    let stdin = io::stdin();

    println!("Enter {} (floating-point):", value_name);

    for line in stdin.lock().lines() {
        let line = line.expect("failed to read line");

        if let Ok(num) = line.trim().parse() {
            return num;
        }
    }

    panic!("Ran out of input");
}


Maintaining Maximum Social Distance So I wrote this simple program to place a certain number of people on a list of seats whilst maintaining maximum social distance.
Given that there is at least one empty seat, calculate the maximum.distance from an empty seat to the closest occupied seat and return the available seat.
How can this be improved to be better
// seats.cpp

#include <iostream>
#include <array>

constexpr int N = 50;

int traverse_right(const std::array<int, N> &seats, int i)
{
    if (i < 0)
        return 0;
    else if(seats[i] == 1)
        return i;
    else return traverse_right(seats, --i);
}

int traverse_left(const std::array<int, N> &seats, int i)
{
    if (i >= seats.size())
        return 0;
    else if(seats[i] == 1)
        return i;
    else return traverse_left(seats, ++i);
}

int get_available_seat(const std::array<int, N> &seats)
{
    int max = 0;
    int right_distance = 0;
    int left_distance = 0;
    int available_seat = -1;

    for(size_t i = 0; i != seats.size(); ++i)
    {
        if(seats[i] == 0)
        {
            right_distance = abs(i - traverse_right(seats, i));
            left_distance = abs(i - traverse_left(seats, i));
        }
        if(left_distance > max && right_distance > max)
        {
            if(left_distance <= right_distance && left_distance != 0)
                max = left_distance;
            else
                max = right_distance;
            available_seat = i;
        }
    }
    std::cout << "Maximum distance to available_seat is " << max << '\n';
    return available_seat;
}

void display(const std::array<int, N> &seats)
{
    for(const auto x : seats)
        std::cout << x << " ";
    std::cout << '\n';
}
int main()
{
    std::array<int, N> seats{};
    for(int i = 0; i < 12; ++i)
    {
        int available_seat  = get_available_seat(seats);
        seats[available_seat] = 1;
    }
    display(seats);
}

The code works as expected and though I didn't perform the check, user would need to test if there is an available seat before placing.

One idea would be to maintain a distance to the nearest seat, rather than simply an occupancy list (you can recover the same information since seats with zero distance are occupied). Then finding the seat with the furthest distance and updating the list reduces to:

finding the maximum element in the array
Traversing left and right from that element and updating the distances until the stored distance is already less than the distance to the newly added seat.

One more code review comment:
It may be nicer to either (1) template these functions on the parameter N, rather than declaring it at the beginning, so that they can be called later on a different sized array or (2) use std::vector so that the size of the seats container doesn't need to be known at compile time.


Save multiple group permissions in one go The method SaveGroupStepPermissions(...) is a database API class.
If an error occurs within the method the exception is trapped. Whether this is good design or not is not the scope of this question as I need to adhere to product guidelines.

I feel that this method is to complex and too lenghty. The obvious solution is to break out code into new functions, but I'm not sure how to do that in a way that is logical.

Important! If an error occurs the method should stop processing and return. That means both foreach loops must be exited.

Note: Input validation is performed in InsertGroupStepPermission() and UpdateGroupStepPermission().

Feel free to suggest improvements to code and comments beside the main question!

public static List<EventItem> SaveGroupStepPermissions(string connectionString, int companyID, List<Group> groups, List<GroupStepPermissions> gsp)
{
    var result = new List<EventItem>();
    result.Add(new EventItem("Insert Step permissions"));
    result.Add(new EventItem("Update Step permissions"));
    try
    {
        using (SqlConnection conn = new SqlConnection(connectionString))
        {
            conn.Open();
            using (SqlTransaction tran = conn.BeginTransaction("ProductName.GSP.cid" + companyID.ToString()))
            {
                foreach (Group group in groups)
                {
                    foreach (GroupStepPermissions stepPermission in gsp)
                    {
                        if (stepPermission.RowID == 0) //new permission that does not exist in the database. (INSERT)
                        {
                            try
                            {
                                InsertGroupStepPermission(conn, tran, stepPermission, companyID, group.ID);
                                result[0].SubEvents.Add(new EventSubItem("Added rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.OK, "Insert OK", stepPermission));
                            }
                            catch (Exception x)
                            {
                                result[0].SubEvents.Add(new EventSubItem("Failed to add rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.Failed, x.Message, stepPermission));
                                throw;
                            }
                        }
                        else //Existing stepPermission. (UPDATE)
                        {
                            try
                            {
                                UpdateGroupStepPermission(conn, tran, stepPermission, companyID, group.ID);
                                result[1].SubEvents.Add(new EventSubItem("Updated rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.OK, "Update OK", stepPermission));
                            }
                            catch (Exception x)
                            {
                                result[1].SubEvents.Add(new EventSubItem("Failed to update rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'", StatusEnum.Failed, x.Message, stepPermission));
                                throw;
                            }
                        }
                    }
                } //End outer for
                tran.Commit(); //important :)
            }
        }
    }
    catch(Exception x)
    {
        var eItem = new EventItem("Failed to save step permissions");
        eItem.SubEvents.Add(new EventSubItem("Error: " + x.Message));
        result.Add(eItem);
    }
    return result;
}

There's no need for fairly meaningless variable names like gsp, conn, tran, eItem,...

And result is equally bad, since it doesn't tell me what it contains.

Exception x is... unexpected. ex or exc is usually used.

You use var, but not consistently. Why SqlConnection conn, Group group, etc.?

I'm sure the .ToString() isn't necessary in "ProductName.GSP.cid" + companyID.ToString() -- unless you're coding to an old version of .NET.

Comments shouldn't tell me what is happening, but why. //End outer for and //important :) don't tell me anything useful.

Meanwhile //new permission that does not exist in the database. (INSERT) tells me that if (stepPermission.RowID == 0) isn't perhaps the best way to check if you need to do an insert or an update.

An enum shouldn't be called StatusEnum. Why not simply Status?

GroupStepPermissions is a class name, yet it seems to represent a single item. That's a big no-no.

Avoid concatenation:

"Added rights for '" + group.Name + "' in step '" + stepPermission.StepTitle + "'"

This is why string.Format() was invented.

Now, let's look at the whole method. You have four parameters, which is bordering on getting hard to maintain. Consider a class that contains companyID, groups and gsp. Note that I leave connectionString out of it; quite frankly I find it odd to see you pass a connectionstring to a method.

There's a lot of semi-duplicate logic, especially in catch (Exception x) in the insert or update. Move that to a method and call it with the required parameters.

I'd also be inclined to move everything inside foreach (GroupStepPermissions stepPermission in gsp) to a separate method, if only to reduce all those indentations.

I don't see the point in doing this:

result.Add(new EventItem("Insert Step permissions"));
result.Add(new EventItem("Update Step permissions"));

...if that results in having to do this:

result[0].SubEvents.Add

Name one of them insertEventItem and the other updateEvenItem and only compile the result list at the end, e.g.

return new List<EventItem>{ insertEventItem, updateEvenItem };


Database design for a school system This system has to manage students, teachers, staff and grading. This is for production and is not a school assignment. As such, please let me know if I can improve on any aspect. :)

My main concern is automation. I'd like my software to be able to run regardless if I still exist.

I'm using SQLite as the database:

create table User
(
ID integer primary key autoincrement,
Username string,
Password string
);

create table Area
(
ID integer primary key autoincrement,
Name string
);

create table Subject
(
ID integer primary key autoincrement,
Name string,
Abbreviation string,
IDArea integer references Area(ID)
);

create table Level
(
ID integer primary key autoincrement,
Name string,
Principle string
);

create table Grade
(
ID integer primary key autoincrement,
Name string,
IDLevel integer references Level(ID),
Observation string
);

create table StaffType
(
ID integer primary key autoincrement,
Name string
);

create table Staff
(
ID integer primary key autoincrement,
IDStaffType integer references StaffType(ID),
Name string,
LastNameFather string,
LastNameMother string,
DateOfBirth string,
PlaceOfBirth string,
Sex string,
Carnet string,
Telephone string,
MobilePhone string,
Address string,
FatherName string,
MotherName string,
FatherContact string,
MotherContact string,
FatherPlaceOfWork string,
MotherPlaceOfWork string,
DateOfHiring string,
YearsOfService string,
Formation string,
Specialty string,
Category string,
Salary string
);

create table GradeParalelo
(
ID integer primary key autoincrement,
IDGrade integer references Grade(ID),
IDStaff integer references Staff(ID),
Name string
);

create table Student
(
ID integer primary key autoincrement,
IDGradeParalelo integer references GradeParalelo(ID),
Rude string,
Name string,
LastNameFather string,
LastNameMother string,
DateOfBirth string,
PlaceOfBirth string,
Sex string,
Carnet string,
Telephone string,
MobilePhone string,
Address string,
FatherName string,
MotherName string,
FatherMobilePhone string,
MotherMobilePhone string,
FatherProfession string,
MotherProfession string,
FatherPlaceOfWork string,
MotherPlaceOfWork string,
Observations string
);

create table Attendance
(
ID integer primary key autoincrement,
IDStudent integer references Student(ID),
Attended string,
Date string
);

create table SubjectGrade
(
ID integer primary key autoincrement,
IDGrade integer references Grade(ID),
IDSubject integer references Subject(ID)
);

create table ScoreRecord
(
ID integer primary key autoincrement,
IDSubject integer references Subject(ID),
IDStudent integer references Student(ID),
FirstTrimester integer,
SecondTrimester integer,
ThirdTrimester integer,
FinalGrade integer,
Year string
);

Any glaring room for improvement?

One thing which immediately jumped out to me is this:

LastNameFather string,
LastNameMother string,
FatherName string,
MotherName string,
FatherContact string,
MotherContact string,
FatherPlaceOfWork string,
MotherPlaceOfWork string,

Your design assumes that every student will have exactly 1 mother and exactly 1 father. I can tell you now that will not be the case. Students with divorced parents may have two mothers and two fathers, all of whom will want to have their contact info listed. Some students may have gay or lesbian parents, and thus two fathers or two mothers. Some students may have neither, and instead may have a legal guardian who is neither their father nor mother.

One solution to this would be to have a table for a "person", and link people to each student. Identify whether that person is a father or mother (or non-parental guardian). This will also simplify having siblings: you can have the same mother, father, etc. for multiple students.

For the majority of students, this won't be an issue, but for more than you might think, it will. Do those families and your management a favor by making it easy to handle various family scenarios!