Leak detection simple class Basic idea is to use a Class, with static methods to add and remove references in a static vector, that keeps track of these references, and check that vector upon exit.

The class is detecting intentional leaks that I create, but maybe you can find a case where it does not detect the leak, that I am not seeing

#include <iostream>
#include <stdexcept>
#include <memory>
#include <vector>
#include <cstdlib> // for REM_LEAKt
#include <algorithm> // for std::remove

void checkLeakStack();
class LeakDbg
{
private:
    LeakDbg()
    {
        /// Constructor will only be called once,
        /// since it's a singlton class
        std::atexit( checkLeakStack );
    }

public:
    struct Pair
    {
        std::string name;
        void* ref;
        bool operator==( const Pair &other ) const { return ref == other.ref; }

    };
    static bool locked;
    static std::vector<Pair> stack;
    static LeakDbg& instance()
    {
        static LeakDbg INSTANCE;
        return INSTANCE;
    }

    static void addRef(const std::string& nm, void* ptr)
    {
        stack.push_back(Pair{ nm, ptr });
    }
    static void remRef(void* ptr)
    {
        /// If it's not enabled, it means
        /// it's OK to remove a ref
        if( !LeakDbg::locked ){
            Pair search = Pair{"",ptr};
            std::vector<Pair> vect;
//            std::remove(vect.begin(), vect.end(), search);
            stack.erase(std::remove(stack.begin(), stack.end(), search), stack.end());
        }
    }

};

bool LeakDbg::locked = false;
std::vector<LeakDbg::Pair> LeakDbg::stack = std::vector<LeakDbg::Pair>();

void checkLeakStack()
{
    /// Here the stack should be emoty
    /// you can print or assert if the stack is not empty
    std::cout << "There are " << LeakDbg::stack.size() << " leaks ..." "\n";
    for ( LeakDbg::Pair pair : LeakDbg::stack) {
        const std::string msg = pair.name + " is leaked";
        std::cout << msg << std::endl;
    }
}

Add defines

#define ADD_LEAK(msg, ptr) LeakDbg::addRef(msg, ptr);
#define REM_LEAK(ptr) LeakDbg::remRef(ptr);
#define CREATE_LEAK_DET() LeakDbg::instance();
#define LCK_LEAK_DET(st) LeakDbg::locked = st;

// ADD_LEAK         -> Add it in a class constructor
// REM_LEAK         -> Add it in a class destructor
// CREATE_LEAK_DET  -> Call it once to make sure "std::atexit" is called
// LCK_LEAK_DET     -> If set to "true", all objects destructed after,
//                  -> will be considered a leak

Test

struct Test {
    Test() {
        ADD_LEAK( "Cls",  this )
    }
    ~Test() {
        REM_LEAK( this )
    }

};

int main() {
    CREATE_LEAK_DET()
    Test *obj1 = new Test();
    Test *obj2 = new Test();
    Test *obj3 = new Test();
    delete obj2;
    LCK_LEAK_DET( true )
}

Update 12/12/2019
If anybody is interested, I refactored the code to be reusable and less intrusive. Github

Here are some things that may help you improve your code.

Use the required #includes

The code uses std::string which means that it should #include <string>.  It might compile on your machine because some other header includes that file, but you can't count on that, and it could change with the next compiler update.

Use only necessary #includes

The #include <stdexcept> and #include <memory> lines are not necessary and can be safely removed because nothing from those headers appears to be used here.

Avoid C-style macros

I'd advise not using C-style macros like the ones in this code, preferring either inline functions or even lambdas.  See ES.31 for details.

Consider thread safety

If multiple threads are using this code, there is likely to be a problem because the single shared instance of the std::vector is not protected by a mutex.  I would also recommend renaming the existing locked variable to something like complete or finished to better distinguish what it's doing.

Avoid singletons

A singleton is basically just another way to create global variables, and we don't like global variables much because they make code linkages much harder to see and understand.  See I.3 for more on that. In this case, since you already have two global variables, much of the complexity can easily be avoided by simply using a namespace instead of a class.  Here's one way to do that which eliminates the need for instance and CREATE_LEAK_DET:

namespace LeakDbg
{
    struct Pair
    {
        std::string name;
        void* ref;
        bool operator==( const Pair &other ) const { return ref == other.ref; }

    };
    static bool locked = false;
    static std::vector<Pair> stack;

    static void addRef(const std::string& nm, void* ptr)
    {
        stack.emplace_back(Pair{ nm, ptr });
    }
    static void remRef(void* ptr)
    {
        if( !LeakDbg::locked ){
            stack.erase(std::remove(stack.begin(), stack.end(), Pair{"",ptr}), stack.end());
        }
    }
    void checkLeakStack()
    {
        std::cout << "There are " << LeakDbg::stack.size() << " leaks ..." "\n";
        for ( LeakDbg::Pair pair : LeakDbg::stack) {
            std::cout << pair.name << " is leaked\n";
        }
    }
    static const bool registered{std::atexit( checkLeakStack ) == 0};
}

Consider the user

The current code requires that the user explicitly instruments the code, which seems a bit intrusive.  Here's an alternative approach the modifies things just slightly, using the Curiously Recurring Template Pattern, or CRTP for short.  First we isolate the leak detector bits into a templated class.

template <typename T>
struct LeakDetector {
    LeakDetector() {
        LeakDbg::addRef(typeid(T).name(), this);
    }
    ~LeakDetector() {
        LeakDbg::remRef(this);
    }
};

Now to use it is much simpler than before.  No ugly macros are required and we only need to add one simple thing to the declaration of the class to be monitored:

struct Test : public LeakDetector<Test>
{
    Test() {
    }
    ~Test() {
    }
};

An even less intrusive approach might be to override new and delete as outlined in this question.

Consider alternatives

Leak detection is a worthwhile thing to do, since many C++ bugs stem from that kind of error.  However, there are already a number of existing approaches to this, some of which may already be installed on your computer.  There is, for example the useful valgrind tool.  If you're using clang or gcc and have the libasan library installed, you can get a very nice runtime printout.  Just compile the code with

g++ -g -fsanitize=address myprogram.cpp -o myprogram

Then at runtime, a memory leak report might look like this:

There are 2 leaks ...
Cls is leaked
Cls is leaked

=================================================================
==71254==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7fe67c2c69d7 in operator new(unsigned long) (/lib64/libasan.so.5+0x10f9d7)
    #1 0x4057a6 in main /home/edward/test/memleak/src/main.cpp:97
    #2 0x7fe67bcbb1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7fe67c2c69d7 in operator new(unsigned long) (/lib64/libasan.so.5+0x10f9d7)
    #1 0x405774 in main /home/edward/test/memleak/src/main.cpp:95
    #2 0x7fe67bcbb1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

SUMMARY: AddressSanitizer: 2 byte(s) leaked in 2 allocation(s).


Query performance of a SELECT statement using WHERE and NOT IN Can anyone help improve the performance of my query?  When I run it, it locks the database for 5 to 10 minutes.

SELECT
    u.username,
    u.email,
    u.created_at,
    p.firstname,
    p.lastname,
    p.address,
    p.address_2,
    p.city,
    p.country,
    p.state,
    p.zip,
    p.phone,
    p.phone_2,
    u.last_ip,
    u.last_login_at,
    u.auto_login,
    u.registration_page,
    s.product_name
    FROM
    users AS u
        Left Join subscriptions AS s ON u.id = s.user_id
        Left Join profiles AS p ON u.id = p.user_id
    where u.registration_page='Chris_Cleanse' and
        u.id not in (select user_id from goal_sheets) and
        u.id not in(select user_id from sheet_user_popup_not_adam) and
        s.expired=TRUE ORDER BY u.id DESC;

Here is the output of EXPLAIN SELECT:

I don't see anything unreasonable about the query itself.

However, if I'm interpreting the output of EXPLAIN correctly, the anti-join with sheet_user_popup_not_adam is being done using a full table scan.  Run

SHOW INDEXES FROM sheet_user_popup_not_adam;

If no index exists on the user_id column, then run

CREATE INDEX sheet_user_popup_not_adam_user_id ON sheet_user_popup_not_adam (user_id);

Maybe that should be CREATE UNIQUE INDEX … instead, if appropriate.  Hopefully the performance should improve after you create the index.


Divide and conquer approach in search for max element, take 2 Adopted some suggestions from the answer of my previous post. Here is the code:

trait MaxElem<T> {
    fn max_elem<'a>(&'a self) -> &'a T;
}

impl<T> MaxElem<T> for [T] where T: Ord {
    fn max_elem<'a>(&'a self) -> &'a T {
        max_elem_helper(self, 0, self.len())
    }
}

fn max_elem_helper<'a, T>(array: &'a [T], left: usize, right: usize) -> &'a T
    where T: Ord
{
    if right - left == 1 {
        return &array[left];
    }
    let mid = (left + right) / 2;
    let max1 = max_elem_helper(array, left, mid);
    let max2 = max_elem_helper(array, mid, right);
    if max1 > max2 {
        max1
    } else {
        max2
    }
}

#[cfg(test)]
mod test {
    use max_elem::MaxElem;
    #[test]
    fn test_max_elem() {
        let array = [11, 2, 9, 1, 3, 88];
        let m = array.max_elem();
        assert_eq!(*m, 88);
    }
}

All suggestions are welcome.

The code panics with a stack overflow if you provide an empty array:

#[test]
fn test_empty() {
    let array: [u8; 0] = [];
    array.max_elem();
}

thread 'test::test_empty' has overflowed its stack
fatal runtime error: stack overflow
error: Process didn't exit successfully: `/private/tmp/max/target/debug/max-426b0349be06fd52` (signal: 6, SIGABRT: process abort signal)

The tests don't work unless the crate is named max_elem. Instead, use use super::MaxElem instead.
You could choose to embed the helper method inside of the other function, giving a natural scoping (and preventing the need for the longer name):

impl<T> MaxElem<T> for [T]
    where T: Ord
{
    fn max_elem<'a>(&'a self) -> &'a T {
        fn helper<'a, T>(array: &'a [T], left: usize, right: usize) -> &'a T
            where T: Ord
        {
            // ...
        }

        helper(self, 0, self.len())
    }
}

Instead of passing in the entire slice and left / right; try splitting the slice into smaller parts via split_at. Then the slice itself tracks the beginning and the end.
The function cmp::max exists.

Overall, there's a nice functional implementation that weighs-in at about 20 lines.

use std::cmp;

trait MaxElem<T> {
    fn max_elem<'a>(&'a self) -> Option<&'a T>;
}

impl<T> MaxElem<T> for [T] where T: Ord {
    fn max_elem<'a>(&'a self) -> Option<&'a T> {
        let len = self.len();

        match len {
            0 | 1 => self.first(),
            _ => {
                let mid = len / 2;
                let (left, right) = self.split_at(mid);

                let left = left.max_elem();
                let right = right.max_elem();

                match (left, right) {
                    (Some(a), Some(b)) => Some(cmp::max(a, b)),
                    _ => left.or(right)
                }
            }
        }
    }
}

#[cfg(test)]
mod test {
    use super::MaxElem;

    #[test]
    fn test_max_elem() {
        let array = [11, 2, 9, 1, 3, 88];
        assert_eq!(Some(&88), array.max_elem());
    }

    #[test]
    fn test_empty() {
        let array: [u8; 0] = [];
        assert_eq!(None, array.max_elem());
    }
}


Ackermann function in Rust Using the algorithm at the end of this research paper, I've written an extremely fast 1 Ackermann function in Rust. Can the implementation be made faster?

Thoughts:

Using a struct makes a lot of sense, but it's weird to have to Some(Box::new(v2)). Can this be avoided in some way?
Are for loops the best way of doing the v transformations? Would (1..(i + 1)).fold(Record::new(1, 0, None), |v, k| a_use_p(k - 1, 1, v)) be more "Rustic"? It seems slightly faster, but that's hard to tell.
Is there a way to parallelize this? Or any part of it?

Thanks.

extern crate time;

use time::PreciseTime;

fn main() {
    let m = 3;

    for n in 1..20 {
        let s = PreciseTime::now();
        let res = fast_ackermann(m, n);
        let e = PreciseTime::now();
        println!("a_opt(3, {}) -> {} took {}", n, res, s.to(e))
    }
}

#[derive(Clone)]
struct Record {
    result: u64,
    previous_result: u64,
    cache: Option<Box<Record>>,
}

impl Record {
    fn new(result: u64, previous_result: u64, cache: Option<Box<Record>>) -> Record {
        Record { result, previous_result, cache }
    }
}

fn fast_ackermann(m: u64, n: u64) -> u64 {
    let mut cache = Record::new(1, 0, None);
    for m_builder in 0..m {
        cache = ack_with_incrementalization(m_builder, 1, cache)
    }
    for n_builder in 1..(n + 1) {
        cache = ack_with_incrementalization(m, n_builder, cache)
    }
    cache.result
}

fn ack_with_incrementalization(m: u64, n: u64, current_cache: Record) -> Record {
    if m == 0 {
        Record::new(n + 1, 0, None)
    } else if n == 0 {
        let mut new_cache = Record::new(1, 0, None);
        for m_builder in 0..m {
            new_cache = ack_with_incrementalization(m_builder, 1, new_cache);
        }
        new_cache
    } else if n == 1 {
        let cache_result = current_cache.result;
        let mut new_cache = current_cache;
        for n_builder in 2..(cache_result + 1) {
            new_cache = ack_with_incrementalization(m - 1, n_builder, new_cache);
        }
        Record::new(new_cache.result, cache_result, Some(Box::new(new_cache)))
    } else {
        let cache_result = current_cache.result;
        let mut new_cache = *current_cache.cache.unwrap();
        for n_builder in (current_cache.previous_result + 1)..(current_cache.result + 1) {
            new_cache = ack_with_incrementalization(m - 1, n_builder, new_cache);
        }
        Record::new(new_cache.result, cache_result, Some(Box::new(new_cache)))
    }
}

1 With m = 3, n = 20, the code takes 0.95 seconds. With n = 30 it takes 16 minutes. In the paper, n = 20 takes 5.05 seconds and n  =30 takes 87 minutes. A version with an intermediate cache (in Rust), n = 20 took 2.1 seconds and n = 25 (not 30 like the others) took 39 minutes.

I realized that the nested Records never go more than 3 deep (outermost, middle, innermost), which means it could be rewritten to use primitive arrays.

Comparison 1:

n: 20
fas_ack -> 8388605 took PT0.399960323S
arr_ack -> 8388605 took PT0.115302129S
n: 25
fas_ack -> 268435453 took PT11.283535003S
arr_ack -> 268435453 took PT3.553065491S
n: 30
fas_ack -> 8589934589 took PT757.605898139S
arr_ack -> 8589934589 took PT233.040801010S

Comparison 2:

n: 20
fas_ack -> 8388605 took PT0.807311828S
arr_ack -> 8388605 took PT0.442201425S
n: 25
fas_ack -> 268435453 took PT35.765848937S
arr_ack -> 268435453 took PT14.004015773S
n: 30
fas_ack -> 8589934589 took PT959.685955407S
arr_ack -> 8589934589 took PT141.390498532S

Updated code:

extern crate time;

use time::PreciseTime;

fn main() {
    let m = 3;

    for n in [20, 25, 30].iter() {
        println!("n: {}", n);
        let s = PreciseTime::now();
        let res = arr_ack(m, *n);
        let e = PreciseTime::now();
        println!("arr_ack -> {} took {}", res, s.to(e))
    }
}

fn arr_ack(m: u64, n: u64) -> u64 {
    let mut cache: [u64; 6] = [0; 6];
    cache[0] = 1;
    for m_builder in 0..m {
        cache = _arr_ack(m_builder, 1, cache)
    }
    for n_builder in 1..(n + 1) {
        cache = _arr_ack(m, n_builder, cache)
    }
    cache[0]
}

fn _arr_ack(m: u64, n: u64, mut cache: [u64; 6]) -> [u64; 6] {
    if m == 0 {
        [n + 1, 0, 0, 0, 0, 0]
    } else if m == 1 {
        [n + 2, 0, 0, 0, 0, 0]
    } else if n == 0 {
        let mut new_cache = [1, 0, 0, 0, 0, 0];
        for m_builder in 0..m {
            new_cache = _arr_ack(m_builder, 1, new_cache);
        }
        new_cache
    } else if n == 1 {
        let previous_result = cache[0];
        for n_builder in 2..(previous_result + 1) {
            cache = _arr_ack(m - 1, n_builder, cache);
        }
        [cache[0], previous_result, cache[0], cache[1], cache[2], cache[3]]
    } else {
        let previous_result = cache[0];
        let mut new_cache = [cache[2], cache[3], cache[4], cache[5], 0, 0];
        for n_builder in (cache[1] + 1)..(previous_result + 1) {
            new_cache = _arr_ack(m - 1, n_builder, new_cache);
        }
        [new_cache[0], previous_result, new_cache[0], new_cache[1], new_cache[2], new_cache[3]]
    }
}


Optimize Javascript function with many (maybe?) loops inside I have the following function using in my application:

applyFormSectionIdsToControls: function(formSections) {
    if (!formSections.length) return;

    var formSectionIds = [];

    for (var i = 0, ii = formSections.length; i < ii; i++) {
        formSectionIds.push(parseInt(formSections[i].id));
    };

    for (var i = 0, ii = formSections.length; i < ii; i++) {
        var affectedWidgets = $.grep(application_widgets, function(item) {
            return (($.inArray(item.widget_key, ['nextbutton', 'dropdown', 'radiobuttons']) >= 0) && (item.parent_id == formSections[i].id));
        });

        for (var j = 0, jj = affectedWidgets.length; j < jj; j++) {
            if (affectedWidgets[j].widget_key == 'nextbutton' && (typeof affectedWidgets[j].settings.default_next_step != 'undefined' && affectedWidgets[j].settings.default_next_step > 0)) {
                var _next_step = parseInt(affectedWidgets[j].settings.default_next_step);

                if ($.inArray(_next_step, formSectionIds) == -1) {
                    affectedWidgets[j].settings.default_next_step = formSectionIds[_next_step - 1];
                }
            }
            if (affectedWidgets[j].widget_key == 'radiobuttons' || affectedWidgets[j].widget_key == 'dropdown') {
                for (var k = 0, kk = affectedWidgets[j].settings.answers; k < kk; k++) {
                    var _next_step = parseInt(affectedWidgets[k].settings.answers[k].next_step);

                    if ($.inArray(_next_step, formSectionIds) == -1) {
                        affectedWidgets[k].settings.answers[k].next_step = formSectionIds[_next_step - 1];
                    }
                }
            }
        }
    };
}

I'm looking for any hints how to refactor it, as I think there are too many for loops.

Look, ma, no loops!
But seriously, if you were concerned about performance I don't think it's a problem of nested loops per se, $.grep in the loop concerns me much more. Especially since you're doing basically the same grep over and over!
After grep, neither item.parent_id nor i are used again. Looks like we can just cycle through all widgets in one $.grep, and that's exactly what we're going to do.
Also, what you're doing with item.widget_key begs for a switch statement - or, at least, having those radiobutton and dropdown typed twice bugs me and begs for a well-placed typo to cause lots of frustration.
I'm not sure I understand exactly what you do with default_next_step, so correctNextStep probably needs some refinement (like, what if next_step - 1 is not a valid forum section index either?) but the code is supposed to work exactly the same as yours. (or not work, hard to say without test cases:D)
const inArray = (needle, haystack) => (-1 != haystack.indexOf(needle));
const parseIntIfDefined = (item) => ('undefined' === typeof(item) ? null : parseInt(item));

const applyFormSectionIdsToControls = function(formSections) {
  if (!formSections.length) return;

  const formSectionIds = formSections.map((section) => parseInt(section.id));
  const correctNextStep = (nextStep) => {
    if (inArray(nextStep, formSectionIds)) return nextStep;
    return formSectionIds[nextStep - 1];
  };

  $.grep(application_widgets, (item) => {
    if (!inArray(parseInt(item.parent_id), formSectionIds)) return;

    switch (item.widget_key) {
      case 'nextbutton':
        const defaultNextStep = parseIntIfDefined(item.settings.default_next_step);
        if (defaultNextStep) {
          item.settings.default_next_step = correctNextStep(defaultNextStep);
        }
      break;
      case 'radiobuttons':
      case 'dropdown':
        item.settings.answers.map(
          (answer) => answer.next_step = correctNextStep(parseInt(answer.next_step))
        );
      break;
    }
  });
}


PHP "web service" and jQuery ajax I've been writing classic ASP.NET web services for a while now, and this is my first time writing a PHP based web service. I'm rather new to PHP as well.

In ASP.NET, you directly call the web method that you wish to be executed, but I'm not sure if PHP has a similar functionality.

Instead I'm calling a PHP page, and to it I'm passing the method name to be executed. Following is my code -

PHP Web Service

<?php
    require_once 'classes/CommonFunctionality.php';

    // Getting the json data from the request
    $json_data = json_decode(file_get_contents("php://input"));
    $response = '';

    // Checking if the data is null..
    if(is_null($json_data))
    {
        $response = json_encode(array("status" => -1, "message" => "Insufficient parmaters!"));
    }
    else
    {
        // Checking if it's null or if it exists...
        if(!property_exists($json_data, "MethodName") || is_null($json_data->MethodName) || function_exists($json_data->MethodName . "()"))
        {
            $response = json_encode(array("status" => 0, "message" => "Invalid function name."));
        }
        else
        {
            // Get method name...
            $methodName = $json_data->MethodName;
            // Call the method...
            $response = $methodName();
        }
    }
    header('Content-type: application/json');
    echo $response;

    /****************************
     * Methods defined for use. *
     ****************************/
    function GetAllCrmUsers()
    {
        // Adding crm users class.
        $userArray = CrmUsers::GetAllUsers();
        return json_encode(array("status" => 200, "value" => json_encode($userArray)));
    }
?>

AJAX call

// URL, Data to send, Call back function.
AjaxCall($("#hdnWbUrl").val() , {MethodName : 'GetAllCrmUsers'}, BindData);

Generic jQuery ajax method

function AjaxCall(url, data, callBack){
    $.ajax({
        url : url,
        type : 'POST',
        data : JSON.stringify(data),
        contentType : 'application/json utf-8',
        dataType : 'json',
        success : function(data, textStatus, jqxhr){
            callBack(data);
        },
        error : function (jqxhr, textStatus, errorMessage){
            console.log(argument);
        }
    })
}

Now my question is if there is a better way to do this, something that I might be unaware of.

First I would like to say, congratulations on stepping out of the Microsoft bubble!

To begin, most PHP functions are very similar (if not exactly the same) to their linux counterparts. Some of them even just directly call their linux counterparts and return the result.  You don't have to know linux shell script to know PHP (or vice versa) but it definitely helps and knowing that there is a deeply connected relationship between the two is a enlightening.

PHP has a few global variables that you should familiarize yourself with:

$_POST: holds any POST data sent with the request
$_GET: holds any GET data sent with the request
$_SESSION: holds the session data

My first recommendation for you is to use the $_POST variable instead of using file_get_contents("php://input").

I have to be honest, in all my years of PHP development, I've never used file_get_contents("php://input") for anything. I didn't even know what that did when I read this post. I had to look it up so that I could answer intelligently.  All the POST data will be nicely, and automatically, placed into the $_POST array for you every single time.

My second recommendation for you is to checkout the PHP function empty().  This can be used on arrays with any key, even if the key doesn't exist, to check either if the key actually exists or if the key has an empty value.  It's great for an initial check on POST data.

So for example, you could replace:

if(!property_exists($json_data, "MethodName") || is_null($json_data->MethodName) || function_exists($json_data->MethodName . "()"))

with

if(empty($_POST['MethodName']))

Pretty neat right?

The only caveat with this function is that it will return true not only if the array index doesn't exist, but also if the value at the array index evaluates to false ("", 0, array(), null, etc...).   If you need to handle values that evaluate to false differently than you can use isset() either in addition to or instead of empty() which will tell you if the key is set, regardless of whether the value is an empty value or not.

So basically, $_POST, empty(), isset() are your bread and butter for form data processing in PHP.  Use them wisely.  :)

You should also check out unset()([http://]php.net/manual/en/function.unset.php), which lets you unset keys in your data arrays, for example, to remove keys that are set but have empty values that you'd rather just not be in the array at all.


Laptop battery level monitor for Linux I come from a C background and am trying to learn Rust. I wrote my first Rust program, to check the battery level on my laptop and warn me if it's low/critical.

It works fine, but I  wrote it in a way I'm familiar with and was wondering if I'm missing out on some features of the Rust language that could make this more portable, more optimal, or safer.

extern crate notify_rust;
use notify_rust::Notification;
use notify_rust::NotificationUrgency;

use std::fs::File;
use std::io::prelude::*;

static CHARGE_NOW: &str =  "/sys/class/power_supply/BAT0/charge_now";
static CHARGE_FULL: &str = "/sys/class/power_supply/BAT0/charge_full";
static STATUS: &str = "/sys/class/power_supply/BAT0/status";
static CHARGING_STR: &str = "Charging";

const CHARGE_WARN: f32 = 15.0;
const CHARGE_CRIT: f32 = 5.0;
const NOTIFICATION_TIMEOUT: i32 = 4000;

fn main() {

    /* If we're charging, just exit */
    if get_str_from_file(STATUS).trim() == CHARGING_STR {
        std::process::exit(0);
    }

    let charge_level: f32 = (get_float_from_file(CHARGE_NOW) /
                             get_float_from_file(CHARGE_FULL)) * 100.0;

    if charge_level < CHARGE_WARN {
        Notification::new()
            .summary("Battery level low!")
            .body(&format!("Battery level: {0:.1$}%", charge_level, 2))
            .icon("dialog-information")
            .urgency(
                if charge_level < CHARGE_CRIT {
                    NotificationUrgency::Critical
                } else {
                    NotificationUrgency::Normal
                }
            )
            .timeout(NOTIFICATION_TIMEOUT)
            .show().unwrap();
    }
}

fn get_str_from_file(file_path: &str) -> String {
    let mut file = File::open(file_path)
        .expect("file not found");
    let mut ret = String::new();
    file.read_to_string(&mut ret)
        .expect("failed to read file");

    ret
}

fn get_float_from_file(file_path: &str) -> f32 {
    get_str_from_file(file_path).trim().parse::<f32>().unwrap()
}

Used crate: notify_rust.

You could replace your get_str_from_file function with the standard read_to_string function:
For instance like so:
use std::fs;

if fs::read_to_string(STATUS).unwrap().trim() == CHARGING_STR) {
    return; //return should be equivalent to std::process::exit(0)
}

This is your implementation:
fn get_str_from_file(file_path: &str) -> String {
    let mut file = File::open(file_path)
        .expect("file not found");
    let mut ret = String::new();
    file.read_to_string(&mut ret)
        .expect("failed to read file");

    ret
}

This is the implementation of std::fs::read_to_string
pub fn read_to_string<P: AsRef<Path>>(path: P) -> io::Result<String> {
    let mut file = File::open(path)?;
    let mut string = String::with_capacity(initial_buffer_size(&file));
    file.read_to_string(&mut string)?;
    Ok(string)
}

Almost identical, except the std lib version is more generic and leaves error handling to the caller. It also uses more efficient memory allocation.
New Type Idiom
The following is not necessarily better, but it is certainly a valid alternative approach.
Instead of relying on using an f32 you could use a wrapper struct around it:
#[derive(Debug, Default, Clone, Copy, PartialEq, PartialOrd)] //not necessary but nice to have
struct ChargeLevel(f32);

This is called New Type Idiom. You could further go ahead and put most of the logic surrounding the charge level into special functions in the ChargeLevel's impl block. Some suggestions:
impl ChargeLevel {
    const WARNING_LEVEL: f32 = 15.0;
    const CRITICAL_LEVEL: f32 = 5.0;

    fn new(now: f32, full: f32) -> Self {
        ChargeLevel((now / full) * 100.0)
    }

    fn is_warning(&self) -> bool {
        self.0 < Self::WARNING_LEVEL
    }

    fn is_critical(&self) -> bool {
        self.0 < Self::CRITICAL_LEVEL
    }
}

You could also combine this struct with the external library stuff:
impl From<ChargeLevel> for NotificationUrgency {
    fn from(level: ChargeLevel) -> Self {
        if level.is_critical() {
            NotificationUrgency::Critical
        } else {
            NotificationUrgency::Normal
        }
    }
}

This would allow you to replace
if charge_level < CHARGE_WARN {
    Notification::new()
        .summary("Battery level low!")
        .body(&format!("Battery level: {0:.1$}%", charge_level, 2))
        .icon("dialog-information")
        .urgency(
            if charge_level < CHARGE_CRIT {
                NotificationUrgency::Critical
            } else {
                NotificationUrgency::Normal
            }
        )
        .timeout(NOTIFICATION_TIMEOUT)
        .show().unwrap();
}

with
if charge_level.is_warning() {
    Notification::new()
        .summary("Battery level low!")
        .body(&format!("Battery level: {0:.1$}%", charge_level.0, 2))
        .icon("dialog-information")
        .urgency(NotificationUrgency::from(charge_level))
        .timeout(NOTIFICATION_TIMEOUT)
        .show()
        .unwrap();
}

A little much for such a small application perhaps, but might give you an example how to write nice abstractions in Rust that don't cost any runtime overhead.
Implementing std::fmt::Display would also make sense for the string formatting, but you get the point.


Determining if a check box has been checked or not I have a block of code that checks to see if a check box is checked or not, and assigns a value to a variable for saving to a MySQL table.  There are approximately 8 of these boxes.

if ($("#amOld").is(":checked")) {
    var amOldGuide=1;
} else {
    var amOldGuide=0;
}
if ($("#folOld").is(":checked")) {
    var folOldGuide=1;
} else {
    var folOldGuide=0;
}
if ($("#tichOld").is(":checked")) {
    var tichOldGuide=1;
} else {
    var tichOldGuide=0;
}
if ($("#bookOld").is(":checked")) {
    var bookOldGuide=1;
} else {
    var bookOldGuide=0;
}
if ($("#nebOld").is(":checked")) {
    var nebOldGuide=1;
} else {
    var nebldGuide=0;
}
if ($("#sterOld").is(":checked")) {
    var sterOldGuide=1;
} else {
    var sterOldGuide=0;
}
if ($("#byteOld").is(":checked")) {
    var byteOldGuide=1;
} else {
    var byteOldGuide=0;
}
if ($("#ingOld").is(":checked")) {
    var ingOldGuide=1;
} else {
    var ingOldGuide=0;
}

I am using something similar for another set of check boxes later on in the project as well.  As it sits, this works, but it is ugly. I feel that there must be a better way to write this that will be easier to maintain, and without having to repeat it later on.  Should I be using a class or objects?

A common question, your question is not so much ugly as it is repetitive. Because of that it violate DRY.

You basically have copy pasted the same structure over and over :

if ($("#XXXOld").is(":checked")) {
    var XXXOldGuide=1;
} else {
    var XXXOldGuide=0;
}

If you turn these vars into the properties of a class, then you could do these assignments in a loop over strings. Something like this:

function Guides(){
  this.update();
}

Guides.prototype.list = ['amOld','folOld','tichOld','bookOld','nebOld','sterOld','byteOld','ingOld'];

Guides.prototype.update = function(){
  for( var i = 0 ; i < this.list.length ; i++ ){
    var guide = this.list[i];
    this[ guide + 'Guide' ] = $("#" + guide ).is(":checked") ? 1 : 0;
  }
}

Or, if you do not wish to expose the list, you could go for something more functional (hat tip to @Warbo):

function Guides(){
  this.update();
}

Guides.prototype.update = function(){
  var list = ['amOld','folOld','tichOld','bookOld','nebOld','sterOld','byteOld','ingOld'],
      self = this;
  list.forEach( function( guide ){
    self[ guide + 'Guide' ] = $("#" + guide ).is(":checked") ? 1 : 0;
  });
}

I also threw in a ternary for good measure since I don't like one liners in curly braces.

You can then access bookOldGuide like this:

var guides = new Guides();
console.log( guides.bookOldGuide );

If you  need to refresh the values inside guides ( because you are about to save data for example ), you simply need to

guides.update();

Finally, there is a performance hit you should be aware of, I am not caching any of the jQuery lookups. You could have update create a cache of the lookups if performance becomes an issue.


Calculating distance and angle between two points, C# program I like to think I'm an intermediate programmer, but I learned to code with the Unity engine so I'm still getting used to not using it, and I've never had someone review my code before. I don't know any other programmers, most people in my town don't even have a computer. I'd appreciate it greatly if you'd be so kind and tell me what you think of my little script here. Rather, be brutal if you want.

My script works fine, but I have some concerns, like the readability of calling a Boolean method with a bunch of parameters in the condition of a while loop for example.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Course_Assignment
{
class DistanceCalculator
{
    /// <summary>
    /// This class calculates the distance between two points and the angle of the slope.
    /// </summary>

    static void Main(string[] args)
    {
        float point1X = 0;
        float point1Y = 0;
        float point2X = 0;
        float point2Y = 0;

        float deltaX = 0;
        float deltaY = 0;

        double distance = 0;
        double angle;

        Console.WriteLine("Welcome. This program calculates the distance \nbetween two points as well as their angle.\nPlease type the prompted values.");

        //GetPoints() defines all the point coordinates via user input and
        //returns true if the data entered was invalid.
        NewPoints : while (GetPoints(ref point1X, ref point1Y, ref point2X, ref point2Y))
        {
            Console.WriteLine("Invalid data has been entered. Try again.");
        }

        //Getting the change in x and y from point 1 to 2.
        deltaX = point2X - point1X;
        deltaY = point2Y - point1Y;

        //Calc() uses the Pythagorean Theorem to calculate and return
        //distance, and calculates angle in degrees by multiplying the
        //tangent of deltaX and deltaY.
        distance = Calc(deltaX, deltaY, out angle);

        //Displaying calculations
        Console.WriteLine("\nDistance: {0:N3}\nAngle: {1:N3} degrees\nWould you like to calculate new points? (y,n)", distance, angle);

        //User decides whether or not to do new calculations.
        string newCalc = Console.ReadLine().ToLower();

        if (newCalc.Equals("y") || newCalc.Equals("yes"))
        {
            goto NewPoints;
        }
        else return;
    }

    static bool GetPoints(ref float newPoint1X, ref float newPoint1Y, ref float newPoint2X, ref float newPoint2Y)
    {

        bool invalid = false;

        //Prompts user for coordinates and sets the cursor in front of the
        //prompt for neatness, then checks validity of user input
        Console.WriteLine("\nPoint 1\nX Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint1X))
        {
            invalid =  true;
            return invalid;
        }
        Console.WriteLine("Y Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint1Y))
        {
            invalid = true;
            return invalid;
        }

        Console.WriteLine("Point 2\nX Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint2X))
        {
            invalid = true;
            return invalid;
        }
        Console.WriteLine("Y Coordinate: ");
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (!float.TryParse(Console.ReadLine(), out newPoint2Y))
        {
            invalid = true;
            return invalid;
        }

        return invalid;
    }

    static double Calc(float deltaX, float deltaY, out double angle)
    {
        double newDistance = Math.Sqrt((deltaX * deltaX) + (deltaY * deltaY));
        angle = (Math.Atan2(deltaY, deltaX)) * (180 / Math.PI);
        return newDistance;
    }
}
}

You should avoid using goto statements. Not because it took me a minute to recall what NewPoints : syntax even means, but because goto makes it extremely hard to follow the execution flow. Cases where goto makes code more readable are rare, and yours is not on of those. Just use another while (or do while) loop instead.
Calc is a poor name for a method, because it doesn't say anything about what sort of calculation it performs. It calculates distance you say? Name it CalcDistance then. Oh, wait, it also calculates angle! Maybe CalcDistanceAndAngle then? That doesn't look right... Which is an indication, that you probably need two separate methods: one to calculate angle and one to calculate distance (related to SRP).

var distance = CalcDistance(...);
var angle = CalcAngle(...);

There is a lot of copy-pasting in GetPoints method. You should probably have a method to query single value. For example

static float RequestValue(string message)
{
    float value;
    while (true)
    {
        Console.WriteLine(message);
        Console.SetCursorPosition(14, Console.CursorTop - 1);
        if (float.TryParse(Console.ReadLine(), out value)) break;
        Console.WriteLine("Invalid data has been entered. Try again.");
    }
    return value;
}

Then you can just initialize individual points:

var point1X = RequestValue("\nPoint 1\nX Coordinate: ");

I am pretty sure you don't need to move cursor, if you call Console.Write instead of Console.WriteLine.
You are using both floats and doubles in your calculations. You should probably use one or the other.
else return; is unnecessary.
I think it would make more sense for Calc to take actual points as input instead of "delta"-s. Meaning you should move:

//Getting the change in x and y from point 1 to 2.
deltaX = point2X - point1X;
deltaY = point2Y - point1Y;

inside Calc's body.


Bubble sort class in Java I have written this class which sorts the array using bubble sort. Please let me know how can I improve it. (I don't like recurrent code in if/else statement)

public class BubbleSort {

public static void numbers(int[] array){
    numbers(array, '<');
}

public static void numbers(int[] array, char direction){
    if(direction == '<'){
        for(int i = 0; i < array.length - 1; i++){
            for(int j = 0; j < array.length - i - 1; j++){

                if(array[j] < array[j + 1]){
                    int tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                }
            }
        }
    }
    else{
        for(int i = 0; i < array.length - 1; i++){
            for(int j = 0; j < array.length - i - 1; j++){

                if(array[j] > array[j + 1]){
                    int tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                }
            }
        }
    }
}

public static void letters(char[] array){
    letters(array, '<');
}

public static void letters(char[] array, char direction){
    if(direction == '<'){
        for(int i = 0; i < array.length - 1; i++){
            for(int j = 0; j < array.length - i - 1; j++){

                if(array[j] < array[j + 1]){
                    char tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                }
            }
        }
    }
    else{
        for(int i = 0; i < array.length - 1; i++){
            for(int j = 0; j < array.length - i - 1; j++){

                if(array[j] > array[j + 1]){
                    char tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                }
            }
        }
    }
}
}

Bubble Sort has an inefficient worst case.  In general, you could speed things up by switching from this \$O(n^2)\$ sort to one of the \$O(n \log n)\$ sorts, e.g. Quicksort.  Simplest would be to just use Arrays.sort but perhaps this is a programming exercise (tagging as reinventing-the-wheel would let reviewers know that).

        for(int i = 0; i < array.length - 1; i++){
            for(int j = 0; j < array.length - i - 1; j++){

                if(array[j] < array[j + 1]){
                    int tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                }
            }
        }

The one thing that Bubble Sort can do well is handle sorted input efficiently in \$O(n)\$ time.  But you don't do that.

        for (int i = 0; i < array.length - 1; i++) {
            boolean bubbled = false;
            for (int j = 0; j < array.length - i - 1; j++) {
                if (array[j] < array[j + 1]){
                    int tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                    bubbled = true;
                }
            }

            if (!bubbled) {
                return;
            }
        }

Now it will return after a single pass if the input is sorted.  If the input is almost sorted, it may return on the second or third pass.

You can save a calculation an outer loop iteration if you're really trying to optimize.

        for (int i = array.length - 1; i > 0; i--) {
            boolean bubbled = false;
            for (int j = 0; j < i; j++) {
                if (array[j] < array[j + 1]){
                    int tmp = array[j];
                    array[j] = array[j+1];
                    array[j+1] = tmp;
                    bubbled = true;
                }
            }

            if (!bubbled) {
                return;
            }
        }

Now you don't have to calculate array.length - 1 - i on every outer iteration.

Note that a naive compiler might actually calculate that on each inner iteration in the original code.


Number of ways to make change for an amount Task

Write a program that, given the amount to make change for and a list of coins prints out how many different ways you can make change from the coins to STDOUT.

My approach

The number to make change for is N. Take one coin C and sum the number of ways to make N - C with the other coins. Add the number of ways to make N - 2C with the other coins, etc, for all N - XC > 0 where X is the number of C coins used. Do that recursively. Memoize it so it's effectively dynamic programming.

Code

(ns hackerrank.core
  (:require [clojure.string :as string]))

(defn get-ways
  [n coins]
  (if (= n 0)
    1
    (if (= (count coins) 0)
      0
      (let [[coin & other-coins] coins]
        (reduce
          #(+ %1 (get-ways (- n (* coin %2)) other-coins))
          (if (= (mod n coin) 0) 1 0)
          (range 0 (/ n coin)))))))

(def get-ways-memoized
  (with-redefs [get-ways (memoize get-ways)] get-ways))

(let
  [n (Integer/parseInt (nth (string/split (read-line) #"\s+") 0))
   coins-line (read-line)
   coins (if (= "" coins-line)
           []
           (map #(Integer/parseInt %) (string/split coins-line #" ")))]
  (println (get-ways-memoized n coins)))

Questions

Any Clojure style / best practices, I'm mostly doing this to learn Clojure.
Any ideas why this wouldn't pass the timing tests on HackerRank. I've checked that the memoized function runs faster than the non-memoized one and AFAIK apart from the large stack resulting from no tail call optimization it should be equivalent to an iterative approach with a matrix storing the results.
Is there a way to do this manually, bottom-up rather than using memoize? Pseudo-code or a hint would be great as I'd like to try to implement it myself. Intuitively it feels like there can't be a bottom-up approach because I don't know in advance which sub-problems need to be solved.

Any Clojure style / best practices

Your nested if statements in get-ways might be nicer as a cond:

(defn get-ways
  [n coins]
  (cond
    (= n 0) 1
    (= 0 (count coins)) 0
    :else (let ...)))

There may be other things, but I'm not a clojure pro.

Why it won't pass the timing tests on HackerRank.

I suspect your with-redefs isn't doing what you expect.  with-redefs will redefine get-ways for the duration of the with-redefs block.  But all you're doing in that block is returning the re-defd function.  So your initial call will call the memoized function, but every recursive call just calls the orignal function, so you lose all the benefit of memoizing in the first place.

You may be better off wrapping your (println (get-ways-memoized n coins))) in with-redefs, or maybe just redfining get-ways to always be memoized:

(def get-ways
  (memoize (fn
    [n coins]
    ...)

Is there a way to do this manually.

Not sure, I'll leave that to someone else to try and answer.


NSArray and NSNumber-int conversions I have a textView which is displaying a string. I also have an array which keeps track of where every line begins, it stores the "start index" or NSRange.location for every line.

However, when text is inserted on one line, the start indexes of every line afterwards change. So I'm doing this:

 //i = the linenumber of the modified row
 // self.lineStartIndexes = NSMutableArray
 for (int j = i+1; j < self.lineStartIndexes.count; j++) {
     _lineStartIndexes[j] = @([(NSNumber *)[self.lineStartIndexes objectAtIndex:j] intValue]+text.length);
 }

This takes 12ms on a large text file, and it feels like it could go a lot faster without all NSNumber-conversions. I have thought about using C arrays, but I don't know the size of the array and everything became very complicated to me. What should I do to optimize this?

You could consider creating a class that holds the indices for a larger amount of text (for example a paragraph or a section or a subsection in the text). That class would have the absolute index for where in the file it begins and relative indices within the paragraph/section each line begins.

Whenever a new line is added or removed only the lines within that paragraph/section would have to be updated and the absolute start index of all the following sections as well.

(I don't know what text you write or what you use the line starts for).

You could still find a specific line very easily (and probably very fast) by enumerating the absolute indices of the paragraphs/sections and when you find the right section enumerate it's lines.


Possible improvement on join query I have the following query which retrieve all those who have ordered a book_id n°10 and but never ordered the book with id n°1.
http://sqlfiddle.com/#!9/1bb9e8/44
|user_id | book_id |
| 1 | 1 |
| 1 | 2 |
| 1 | 3 |
| 2 | 1 |
| 2 | 10|
| 2 | 9 |
| 3 | 5 |
| 3 | 10|
| 4 | 6 |
| 4 | 7 |
| 4 | 10|
| 8 | 8 |

select u.user_id,o.book_id
from users u
inner join c
  on u.user_id = o.user_id
    and  o.book_id = 10
 where u.user_id not in (
 select 02.user_id
   from orders o2
  where o2.book_id = 1 );

The keyword "except" is not available on MariaDB 10.1
What are the others way can this be written in SQL aside using a CTE that would be efficient ?

By improvement, I suppose you have performance in mind because your queries are fairly basic and not hard to understand.
I have had a look at your SQL fiddle and your tables have no indexes. Absent any other optimization mechanism the database engine will have to perform a full table scan to fetch the wanted rows. Thus, I don't expect much difference between a join or a subselect, or CTE. On a large table, the bottleneck is likely going to be row filtering, on a small table the performance hit should be negligible.
First, consider adding indexes on your tables, for example the user ID field could be a primary key.
Then use the explain command, preferably on a larger dataset for more representative results. For this you could write a script to generate a large bunch of random records.


How to make a int not be used twice when it randomly generates them? I've coded a simple quiz game for Android, and currently Im having troubles with making questions not appear after they've been shown, i.o. I dont want it to ask me the same question twice..

This is the method Im using

 private void QBegin() {
   /*
    * Gets a random question
    */
    question = (TextView) findViewById(R.id.question);
    String[] types = { "Question one",
            "Question two",
            "Question three",
            "Question four",
            "Question five"};
    Random random = new Random();
    int qType = random.nextInt(types.length);
    question.setText(types[qType]);
    getAnswers(qType); //gets four answers
}

Im not sure if this will work but, what if I add something like

Edit : Doesn't work..

    int i = 0;
    ArrayList<Integer> list = new ArrayList<Integer>();
    list.add(qType);
    i++;
    if(list.contains(qType) && i != types.length + 1){
        return;
    } else {
        answerCounter.setText("Hit the bricks pal, you're done..");
    }

On http://stackoverflow.com got told to , add smth like:

    ArrayList<Integer> list = new ArrayList<Integer>();
    list.add(types.length);
    Collections.shuffle(list);
    if(!list.contains(qType)){
        // help please, as I have no idea on what I should be doing!
    }

There are basically two different ways of doing this efficiently:

Put the questions in a list
Shuffle the list
Loop through the list and show the questions

or:

Put the questions in a list
Pick a question to show by random
Remove that question from the list
Repeat until the list is empty


Random maths quiz with sorting import random
import operator
import pickle

classname = ""
while True:
    classpick = input("Enter class name (\"A,B,C\") ").upper()
    try:
        if classpick in ["A","B","C"]:
            classname = "class"+classpick+".txt"
            break
        else:
            raise ValueError
    except ValueError:
        print("Invalid value. Please enter a letter A,B or C.")
        break

with open(classname + ".txt", "rb") as handle:
    data = pickle.load(handle)

def test():
    num1 = random.randint(1, 10)
    num2 = random.randint(1, num1)

    ops = {
        '+': operator.add,
        '-': operator.sub,
        '*': operator.mul,
    }

    ops_keys = list(ops.keys()) ##=> ['+', '*', '-']
    ops_key_choice = random.choice(ops_keys)  #e.g. '+'
    operation = ops[ops_key_choice]  #e.g. operator.add

    correct_result = operation(num1, num2)
    question = ("Q{}. What is {} {} {}?".format(i+1,num1, ops_key_choice, num2))
    print(question)
    while True:
        try:
            user_answer= int(input("Answer: "))
        except ValueError:
            print ("That is not a valid answer")
            print (question)
        else:
            break

    if user_answer != correct_result:
        print ("Incorrect. The right answer is {}".format(correct_result))
        return False
    else:
        print("Correct!")
        return True
###
def sort_dict(data):
    while True:
        sortpick = str(input('''Enter a number to sort by:
                         \n1. Alphabetically
                         \n2. Highest Score
                         \n3. Highest Average Score'''))
        try:
            if sortpick in ["1","2","3"]:
                break
            else:
                raise ValueError
        except ValueError:
            print("Invalid value. Please enter a number in the range of 1-3.")
        break

    if sortpick == "1":    #Alphabetical
        for x, y in sorted(data.items()):
            print(x,y)

    elif sortpick == "2":    #Highest Score
        for k,v in sorted(((max(data[k]), k) for k in data), reverse=True):
            print('{} : {}'.format(v,k))

    else:   #Highest Average Score
        sort = sorted(data, key=lambda k: sum(data[k]) / len(data[k]), reverse=True)
        for i in sort:
            print("{} : {}".format(i,
                                   round(sum(data[i])/len(data[i]),1 ) ))

username = input("What is your name? ").upper()
print("Hi {}! Welcome to the Primary School Arithmetic quiz...".format(username))

correct_count = 0
question_count = 10

for i in range(question_count):
    if test():
        correct_count +=1

if username in data:
    if len(data[username]) == 3:
        del(data[username][0])
        data[username].append(correct_count)
    else:
        data[username].append(correct_count)
else:
    data[username] = [correct_count]

print("{}: You got {}/{} questions correct. {}".format(
    username,
    correct_count,
    question_count,
    'Great job!' if (correct_count>7) else 'Better luck next time...'
))

with open(classname+".txt", "wb") as handle:
    pickle.dump(data, handle)

print("{}: {}".format(k, v) for k, v in data.items())    #prints whole dict
for k, v in data.items():
    print("{}: {}".format(k, v))
while True:
    try:
        sort_or_not = input("Would you like to sort the class results?? Y/N").upper()
        if sort_or_not.isalpha() == True:
            break
        else:
            raise ValueError
    except ValueError:
        print ("That is not a valid input")
    break

if sort_or_not == "Y":
    print("")
else:
    quit()

sort_dict(data)

The code above is designed to ask users a set of 10 randomized maths questions then save the score to a file and sort the scores.

How can I make this code more efficient?

You should really use more functions,
for a little more to type def fn_name and fn_name() you can get way more readability.
You can also get quite a significant speed up.

For example the code to pick a class name can be wrapped in one.

def pick_class_name():
    classname = ""
    while True:
        classpick = input("Enter class name (\"A,B,C\") ").upper()
        try:
            if classpick in ["A","B","C"]:
                classname = "class"+classpick+".txt"
                break
            else:
                raise ValueError
        except ValueError:
            print("Invalid value. Please enter a letter A,B or C.")
            break

    return classname

This can be improved quite significantly.
The only time that the try will raise a ValueError is when you explicitly raise ValueError.
This basically means that your except should be your else.

def pick_class_name():
    classname = ""
    while True:
        classpick = input("Enter class name (\"A,B,C\") ").upper()
        if classpick in ["A","B","C"]:
            classname = "class"+classpick+".txt"
            break
        else:
            print("Invalid value. Please enter a letter A,B or C.")
            break

    return classname

The above clearly shows the while True is pointless, along with all the breaks.
Instead you could remove the break on the else and change the class_name = into a return.

def pick_class_name():
    while True:
        classpick = input("Enter class name (\"A,B,C\") ").upper()
        if classpick in ["A","B","C"]:
            return "class" + classpick + ".txt"
        else:
            print("Invalid value. Please enter a letter A,B or C.")

You do these try: if: ... else: raise: except: blocks rather than just a simple if ... else,
in a few places, mostly in while loops, that can probably be changed like the above.

You use test in a for loop but rely on the global i to output the correct question number.
This is just bad, first for i in ... is only good if you use that i in a small loop like:

def test(question_number):
    #...
    # Use `question_number` rather than `i`

for i in range(10):
    test(i)

This is as tracking global and local one letter variable names is hard and annoying.
Even more so if you are using an idiom unidiomatically.
So you would want to use question_number rather than i in this case.
You however don't want to change i to question_number in the loop,
as you'll still have to remember global variable.

This would also allow you to change that block of code into a one-liner:

correct_count = sum(map(test, range(question_count)))
correct_count = sum(test(i) for i in range(question_count))

Where if you used the first one in your current code you will get an error or the question number won't change.
And so it's prone to breaking your function depending on how it's used.

Your code is quite hard to read, as you rely on global variables in your functions, and you don't have many functions.
To both make it more efficient and easier to read you should add more functions.


Removing nested for loops to hasten API call response time I've got an MVC APU project that I'm working on to feed data to some JavaScript charts. This beast below takes nearly 1 second of server time to process before it is served to the chart. I know that it's because of the nested for loops.

The chart is a line. By default it shows one data point per month for the last 5 years (one line per year, up to 12 points per line).

The only way I can think to do this is to say for each year, for each month add a point.

Is there a more efficient way to get this done? The resulting JSON looks like this:

{"YearLabel":2011,"MonthProduction":7505.83,"MonthNumber":1}

public string DealsPerMonth(int? BrokerID, int NumberOfYears, int? StartingYear)
    {
        DBNAME db = new DBNAME();

        List<NumberOfDealsPerMonth> dealsPerMonth = new List<NumberOfDealsPerMonth>();

        StartingYear = StartingYear ?? DateTime.Now.Year;

        for (var i = NumberOfYears; i > 0; i--)
        {
            var year = StartingYear - (i - 1);

            var deals = db.VDealParticipations.Where(w => w.IsPrimaryParticipant && w.EffectiveDate.Value.Year == year);

            if (BrokerID != null)
                deals = deals.Where(w => w.BrokerID == BrokerID);

            for (var month = 1; month <= 12; month++)
            {
                if (!(year == DateTime.Now.Year && month > DateTime.Now.Month))
                {
                    var dealCount = deals.Count(w => w.EffectiveDate.Value.Month == month);
                    dealsPerMonth.Add(new NumberOfDealsPerMonth
                    {
                        YearLabel = (int) year,
                        DealCount = dealCount,
                        MonthNumber = month
                    });
                }
            }
        }
        var serializer = new JavaScriptSerializer();
        var value = serializer.Serialize(dealsPerMonth.ToArray());
        return value;
    }

The class that's being used is as follows, although I am probably going to delete it as I found out about anonymous return types and so instead of

select(s => new NumberOfDealsPerMonth {blah = s.bleh}

I can do

select(s => new {blah = s.bleh}

and it does the same thing. But I want to include it anyway just for the sake of more information.

public class NumberOfDealsPerMonth
{
    public int YearLabel { get; set; }
    public int DealCount { get; set; }
    public int MonthNumber { get; set; }
}

I think you will see a performance improvement by using GroupBy.

Let's start by calculating the earliest year we're interested in:

startingYear = startingYear ?? DateTime.Now.Year;
var minYear = startingYear - numberOfYears + 1;

Then filter the rows

var deals = db.VDealParticipations
    .Where(deal => deal.IsPrimaryParticipant &&
                   deal.EffectiveDate.Value.Year >= minYear &&
                   deal.EffectiveDate.Value.Year <= startingYear);
if (brokerID.HasValue)
{
    deals = deals.Where(deal => deal.BrokerID == brokerID);
}

Finally we group the rows by their year/month.

var dealsPerMonth = deals
    .GroupBy(deal => new { deal.EffectiveDate.Value.Year, deal.EffectiveDate.Value.Month })
    .Select(g => new { g.Key.Year, g.Key.Month, Deals = g.Count() });
return new JavaScriptSerializer().Serialize(dealsPerMonth.ToArray());

Note that this does not return months with a deal count of 0.


Prime factorization of a natural number greater than 1 I want to write a program in Java that return a prime factorization of any natural number greater than one.

After spending some time coding and debugging, the program runs fine. Since I am new to Java and programming in general, I don't know how bad my code style and class/method design are. Hence, I am here asking for some tips/advice on what I did wrong and what I could do to improve.

public class FTA_Prime_Factorization {

    /**
     * A method to ask for a integer input from user.
     *
     * @return num: int
     */
    protected static int user_input() {
        int num;
        Scanner inp;
        while (true) {
            inp = new Scanner(System.in);
            System.out.print("Please enter a greater-than-1 positive integer: #");
            if (inp.hasNextInt() && (num = inp.nextInt())>1)
                break;
            System.out.println("Please enter a integer greater than 1!!!");

        }
        return num;
    }

    /**
     * To check whether the parameter is a prime number.
     *
     * @param x: int
     * @return counts == 2: boolean
     */
    private static boolean checkPrime (int x) {
        int counts = 0;
        for (int i = x; i > 0; i--) {
            if (x % i == 0) {
                counts++;
            }
        }
        return counts == 2;
    }

    /**
     * To create an Arraylist of prime number that is less than 10000.
     *
     * @return primeList: Arraylist<Integer>
     */
    private static ArrayList<Integer> primes() {
        ArrayList<Integer> primeList = new ArrayList<>();
        for (int x = 1; x < 10000; x++){
            if (checkPrime(x))
                primeList.add(x);
        }
        return primeList;
    }

    /**
     * To create an Arraylist of prime factorization of the parameter.
     *
     * @param x: int
     * @return primeFactors: Arrarylist<Integer>
     */
    private static ArrayList<Integer> array(int x) {
        ArrayList<Integer> primeFactors = new ArrayList<>();
        ArrayList<Integer> listOfPrime = primes();
        for (int i : listOfPrime) {
            for (int j = x; j % i== 0; j /= i)
                primeFactors.add(i);
        }
        return primeFactors;
    }

    public static void main(String [] args) {
        ArrayList<Integer> temp = array(user_input());
        HashMap<Integer, Integer> counter = new HashMap<>();
        String primeFactors = "";
        for (int element : temp) {
            if (counter.get(element) != null){
                counter.put(element, counter.get(element)+1);
            }else{
                counter.put(element, 1);
            }
        }
        System.out.print("Canonical Form Prime factorization of x = ");
        for (int key : counter.keySet()) {
            primeFactors += counter.get(key) == 1 ? "(prime)"+key + " x " : "(prime)"+key+"^"+counter.get(key) + " x ";
        }
        System.out.println(primeFactors.substring(0, primeFactors.length() - 3));
    }
}

An example output (input = 3612):

Canonical Form Prime factorization of x = (prime)2^2 x (prime)3 x (prime)7 x (prime)43

Method names

You should try to make the name of your methods are descriptive as possible, without being too long:

user_input should first be userInput according to Java naming conventions. But it is difficult to tell what the method does by just looking at its name: it surely involves the user input but we don't know how and why. In this case, the purpose of the method is to retrieve a number to factor. A possible name would be getNumberToFactor or askNumberToFactor, although I would prefer the first variant because it describes what the method does and does not care of how it does it: in the future, you might expand this code and read the number from a file for example; the method's purpose would still be the same, and its name wouldn't change.
checkPrime could be renamed isPrime: generally, boolean-returning methods have a name starting with is (or has).
primes is fine: it returns a list of prime numbers and it is well described that this name. (Perhaps getPrimes() but this is a matter of opinion)
array is misleading. One would assume that it makes an array of something when it fact, this is the prime method making the factorization. It would be better to name it primeFactors and make it clear that this method makes the prime factorization of the given parameter.

Programming to an interface

It is a good idea to program to an interface. This means, trying to avoid the implementing classes and work only on the interfaces. In this case, you are passing ArrayList when you should be passing List:

           //  v--v
private static List<Integer> primes() {
    List<Integer> primeList = new ArrayList<>();
 // ^--^
    for (int x = 1; x < 10000; x++){
        if (checkPrime(x))
            primeList.add(x);
    }
    return primeList;
}

    Map<Integer, Integer> counter = new HashMap<>();
//  ^-^

Separated logic

You have reasonably well separated the code into methods, but the main array is missing its main ingredient. This method is the central-point for making the prime factorization. Therefore, it is that method that should direcly return a Map<Integer, Integer> of the prime factors with their exponent.

Currently, this code is separated in two unrelated blocks:

private static ArrayList<Integer> array(int x) {
    // return list of primes dividing x
}

public static void main(String [] args) {
    ArrayList<Integer> temp = array(user_input());
    HashMap<Integer, Integer> counter = new HashMap<>();
    // process that list and turn it into a Map
}

Note that separating this code actually hides you from an optimization you can make: you don't need to have a list of all primes factors with duplicate values. You can just construct the Map directly, without having that temporary list:

private static Map<Integer, Integer> primeFactors(int x) {
    Map<Integer, Integer> primeFactors = new HashMap<>();
    ArrayList<Integer> listOfPrime = primes();
    for (int i : listOfPrime) {
        for (int j = x; j % i == 0; j /= i) {
            if (primeFactors.get(i) != null) {
                primeFactors.put(i, primeFactors.get(i) + 1);
            } else {
                primeFactors.put(i, 1);
            }
        }
    }
    return primeFactors;
}

If you're using Java 8, you can make this even simpler by replacing the updating of the map with counter.merge(i, 1, Integer::sum);:

private static Map<Integer, Integer> array(int x) {
    Map<Integer, Integer> counter = new HashMap<>();
    ArrayList<Integer> listOfPrime = primes();
    for (int i : listOfPrime) {
        for (int j = x; j % i == 0; j /= i) {
            counter.merge(i, 1, Integer::sum);
        }
    }
    return counter;
}

Avoid concatenating Strings with +=

In the following loop

for (int key : counter.keySet()) {
    primeFactors += counter.get(key) == 1 ? "(prime)" + key + " x "
            : "(prime)" + key + "^" + counter.get(key) + " x ";
}

you are concatenating the primeFactors variables with +=. This is not a good practice because it creates a lot of Strings in memory. It would be better to use a StringBuilder for this task and append each part with append(...).

Also, I don't see the need to print (prime) each time: you know it's a prime already since we're doing the prime factorization.

If you're using Java 8, you can actually use StringJoiner instead: this will also enable you to get rid of the

primeFactors.substring(0, primeFactors.length() - 3)

at the end. This class is designed to join Strings together with a separator:

StringJoiner sj = new StringJoiner(" ");
for (int key : counter.keySet()) {
    sj.add(counter.get(key) == 1 ? key + " x" : key + "^" + counter.get(key) + " x");
}
System.out.println(sj.toString());


Sorting and tagging values in objects Wherever the biggest number is always use the 'big' value

If another number is equal to that number, use the 'big' value

Set the next number in line to 'medium'

If another number is equal to that number, use the 'medium' value

Set the next number in line to 'small'

If another number is equal to that number, use the 'small' value

entries: any { "FirstNames": 6, "Names": 8, "": 6, "Locations": 3, "Others": 2};

let sorted = this.sortByValues(this.entries);

var obj = {};
for (var i = 0; i <= sorted.length; i++) {
    if (Object.keys(obj).length === 0 && obj.constructor === Object)
        obj[sorted[i]] = "big";
    else if (this.entries[sorted[i]] == this.entries[sorted[i-1]] && obj[sorted[i-1]] == "big")
        obj[sorted[i]] = obj[sorted[i-1]];
    else if(this.entries[sorted[i]] < this.entries[sorted[i-1]] && obj[sorted[i-1]] == "big")
        obj[sorted[i]] = "medium";
    else if(this.entries[sorted[i]] == this.entries[sorted[i-1]] && obj[sorted[i-1]] == "medium")
        obj[sorted[i]] = obj[sorted[i-1]];
    else if(this.entries[sorted[i]] < this.entries[sorted[i-1]] && obj[sorted[i-1]] == "medium")
        obj[sorted[i]] = "small";
    else if(this.entries[sorted[i]] == this.entries[sorted[i-1]] && obj[sorted[i-1]] == "small")
        obj[sorted[i]] = obj[sorted[i-1]];
    else
        obj[sorted[i]] = obj[sorted[i-1]];
}

sortByValues(list : any)
{
    return Object.keys(list).sort(function(a,b){return list[a]-list[b]}).reverse());
}

Only two values of the sorted array are needed to decide the resultant tags, so the algo is:

scan all entries and find the maximum and pre-maximum.
scan all entries and tag them accordingly in a new object

Tests show 10x speed-up, but of course it's noticeable only on large number of repetitions.

tagValues(data : any) {
    let keys = Object.keys(data);
    let len = keys.length;
    let max = -Infinity, max2 = -Infinity;
    for (let i = 0; i < len; i++) {
        let value = data[keys[i]];
        if (value > max) {
            max2 = max;
            max = value;
        }
    }
    let tagged = {};
    for (let i = 0; i < len; i++) {
        let key = keys[i];
        let value = data[key];
        tagged[key] = value === max ? 'big' : value === max2 ? 'medium' : 'small';
    }
    return tagged;
}

P.S. As for sortByValues, instead of reversing an array after sorting it, invert the comparison.


Delaying a function call for 30 seconds using sleep() or checking the time in a loop I need a way to delay a function call by 30 seconds on a machine that runs on electric power, not battery (if it matters). I have heard from my colleague not to use sleep, but this post suggests it:

sleep(30) // sleep for 30 sec

Reasons invoked for not using sleep include:

I will violate Single Responsibility Principle.
Abstraction leak, etc.

Not sure why though; I feel it's good because it reduces CPU load and executes important tasks. In the below approach I am trying to just avoid sleep because I am asked not to use it though I am curious to know what a better (best?) approach would be?

void waiting (unsigned int secs)
{
        unsigned int retTime = time(0) + secs;   // Get finishing time.
        while (time(0) < retTime);               // Loop until it arrives.
}

There is no reason to avoid doing sleep(30);. Your colleague's concerns about the single responsibility principle and abstraction leak are unfounded. sleep() is designed to do exactly what you want, so why not use it? Constantly checking the time in a tight loop is a giant waste of computing power for no advantage at all.


Craps game rules and code I'm developing a small craps game in C++, and my C++ skills are a bit rusty.  I really need someone to review my code to ensure that I have correctly implemented the game according to the rules.

Game rules:

  The player or shooter rolls a pair of standard dice

  If the sum is 7 or 11 the game is won
  If the sum is 2, 3 or 12 the game is lost
  If the sum is any other value, this value is called the shooter’s point and he
  continues rolling until he rolls a 7 and loses or he rolls the point again in which
  case he wins

  If a game is won the shooter plays another game and continues playing until he
  loses a game, at which time the next player around the Craps table becomes the
  shooter

My code:

#include <iostream>
#include <ctime>

using namespace std;

bool checkWinning(int roll);

int main(int argc, const char * argv[])
{
    //create player aka shooter

    //create pair of dice
    unsigned int dice1=0;
    unsigned int  dice2 = 0;

    //create roll
    unsigned int roll = 0;

    //create game loop
    while(checkWinning(roll) == true)
    {
        dice1 =  rand() % 6 + 1;
        dice2  = rand() % 6 + 1;
        roll = dice1 + dice2;

        cout<< dice1<<" +"<< dice2<<" = "<<  roll << endl;
        //cout<< checkWinning(2) <<endl;
    }

    return 0;
}

bool checkWinning(int roll)
{
    bool winner  = true;
    if( roll == 2 || roll == 3 || roll == 12)
        return winner= false;
    else
        if(roll == 7 || roll == 11 )
        return winner;
    else
        return winner;
};

Try not to use using namespace std.
The parameters in main() are only necessary if you're executing from the command line.
You're not calling std::srand() nor including <cstdlib>.  However, if you're using C++11, both std::srand and std::rand are not recommended due to certain computational complications (the C++11 pseudo-random number generators can be found under <random>).  But, for a simple program, it may not matter.

In general, here's how to call std::srand():

// casting may just be necessary if warnings are generated
//     that will alert you if there's a possible loss of data
// prefer nullptr to NULL if using C++11
std::srand(static_cast<unsigned int>(std::time(NULL)));

Only include this once, preferably at the top of main().  This is preferred because

It'll help you keep track of it, especially if it'll need to be removed at some point.
If called repeatedly, you'll receive the "same random number" each time.

It's best to keep variables as close in scope as possible.  Here, dice1 and dice2 can be initialized in the while-loop:

unsigned int dice1 = rand() % 6 + 1;
unsigned int dice2 = rand() % 6 + 1;

roll, however, will need to stay where it is so that the loop will work.
The bool-checking can be shortened:

// these are similar
while (checkWinning(roll) == true)
while (checkWinning(roll))

// these are also similar
while (checkWinning(roll) == false)
while (!checkWinning(roll))

checkWinning() takes int roll, but roll is already unsigned int.  They should match.
checkWinning()'s closing curly brace shouldn't have a ;.  It's not a type.
bool winner seems redundant; just return true or false.  Also, the conditions seem a little unclear.  If the sum constitutes a win or a re-roll, how do you specifically distinguish the two?  They both return true.  I'd at least rename the function for clarification.  There's also a Boolean enum, but that may be overkill here (or even unnecessary as there are only two ending outcomes).
There should be a final outcome message, indicating a win or a loss.  Also, you're not giving the player the option to play another game if victorious (and until loss).


Bank account balance, withdraw, deposit: compartmentalize without losing functionality Is there a better way to write this while maintaining its current functionality? (checking balances, updating, subtracting, etc...) If so, where would I begin? I saw a code briefly in class that was for a similar questions and it looked more streamlined.

class Account:

    # constructor
    def __init__(self, name, account_number, balance):
        self.name = name
        self.account_number = account_number
        self.balance = balance

    # returns string representation of object
    def __str__(self):
        return"Account Name: {0}\nAccount Number: {1} \nAccount Balance:\
    ${2:.2f}".format(self.name, self.account_number, self.balance)

    # add given amount to balance
    def deposit(self, amount):
        self.balance += amount

    # subtract amount and fee from balance
    def withdraw(self, amount, fee):
        self.balance = self.balance - amount - fee

if __name__ == '__main__':
    # make 3 objects
    acct1 = Account('Guy Mann', 90453889, 100)
    acct2 = Account('Donald Duck', 83504837, 100)
    acct3 = Account('Joe Smith', 74773321, 100)

    # print
    print(acct1)
    print(acct2)
    print(acct3)

    # deposit and print
    acct1.deposit(25.85)
    acct2.deposit(75.50)
    acct3.deposit(50)
    print(acct1)
    print(acct2)
    print(acct3)

    # withdraw and print
    acct1.withdraw(25.85, 2.50)
    acct2.withdraw(75.50, 1.50)
    acct3.withdraw(50, 2)
    print(acct1)
    print(acct2)
    print(acct3)

This code is already quite compact. Some PEP8 code styling will not hurt. Some spaces here and there, consistency in using quotes in string literals. Also some comments can be turned into docstrings. (and comment like "constructor" is not necessary - everyone who knows Python will recognize that).

However, one thing probably good to make sure: The balance better use proper type, for example, decimal. Float is terribly wrong type for monetary transactions.


Design - prevent multiple nesting in function I have a function written in Groovy which needs to save a record and also return response from the server.
Function is working fine, but I am not happy with the design because I have a problem with multiple nesting.

This is the function:

Map scanAccessRecord(GrailsParameterMap params){

    Map response
    Date currentTime = new Date()
    String readerEthernetAddress = params.readerMac
    Reader reader = Reader.findByDeletedAndEthernetAddress(false, readerEthernetAddress)
    Boolean isAccessIn = (params.isAccessIn == "True")

    if(reader)
    {
        UserSettings userSettings = UserSettings.findByRfidCode(params.rfid.toString())

        if(userSettings)
        {
            if(userSettings.user.company.id == reader.companyId)
            {
                AccessRecord accessRecord = new AccessRecord(reader:reader, userId:userSettings.userId, specificDate:currentTime, isAccessIn: isAccessIn)

                if(accessRecord.save(flush: true))
                {
                    response = [
                            status: '00',
                            message: 'OK',
                            recordId: accessRecord.id,
                    ]
                }
                else
                {
                    response = [
                            status: '60',
                            message: 'error'
                    ]
                }
            }
            else
            {
                response = [status: '40', message: 'USER AND READER ARE NOT FROM SAME COMPANY']
            }
        }
        else
        {
            response = [status: '10', message: 'UNKNOWN USER']
        }
    }
    else
    {
        response = [status: '50', message: 'READER NOT FOUND']
    }

    return response
}

What can I do to make this function "cleaner"?

Use optional instead of null. I wrote an article about avoiding nulls if you are interested.
Use early returns to avoid nesting

If !reader return [status: '50', message: 'READER NOT FOUND']

If !userSettings return...
Rest of the logic

I prefer to avoid nulls,so I would choose to call save with failOnError=true and it will throw exception instead of returning null when save fails.
I assume  you want any exception to return [ status: '60', message: 'error'], if you use save with failOnError=true then the error is handled in one place only.
add validation for params.isAccessIn. If by mistake it is nor true or false, it is better to throw error than do something the client didn't intend to.
Maybe you should be more flexible with params.isAccessIn and compare strings without case sensitivity.


Comparing two objects having two nullable date fields ExpiryDates has two properties PSL_ExpiryDate and MNL_ExpiryDate of type nullable date. I am trying to compare two objects for having same or different values.

Is there a cleaner way to do the same as the code below?

private bool SameValues(ExpiryDates ExpiryDates1, ExpiryDates ExpiryDates2)
    {
        //Assume they are the same value and the look for differences
        bool result = true;

        if (ExpiryDates1.PSL_ExpiryDate.HasValue != ExpiryDates2.PSL_ExpiryDate.HasValue)
        {
            result = false;
        }

        if (ExpiryDates1.MNL_ExpiryDate.HasValue != ExpiryDates2.MNL_ExpiryDate.HasValue)
        {
            result = false;
        }

        if ((ExpiryDates1.MNL_ExpiryDate != null) && (ExpiryDates2.MNL_ExpiryDate != null))
            if (ExpiryDates1.MNL_ExpiryDate.Value != ExpiryDates2.MNL_ExpiryDate.Value)
                result = false;

        if ((ExpiryDates1.PSL_ExpiryDate != null) && (ExpiryDates2.PSL_ExpiryDate != null))
            if (ExpiryDates1.PSL_ExpiryDate.Value != ExpiryDates2.PSL_ExpiryDate.Value)
                result = false;

        return result;
    }

The ExpiryDate class looks like this:

public class ExpiryDates
{
    public DateTime? MNL_ExpiryDate { get; set; }
    public DateTime? PSL_ExpiryDate { get; set; }
}

Nullable<T> already does the hard work for you. Just override Equals on your ExpiryDates class (and GetHashCode):

public override bool Equals(object obj)
{
    var otherDates = obj as ExpiryDates;

    return otherDates != null
        && MNL_ExpiryDate.Equals(otherDates.MNL_ExpiryDate)
        && PSL_ExpiryDate.Equals(otherDates.PSL_ExpiryDate);
}

As pointed out by mjolka in a comment. It would be advisible to implement IEquatable<T> (IEquatable<ExpiryDates>) as well. See the documentation for why it's a useful thing to do.

Further edit... It does say in the comments for IEquatable<T> but you should additionally override == and != e.g.

public static bool operator ==(ExpiryDates left, ExpiryDates right)
{
    if (object.ReferenceEquals(left, null))
    {
        return object.ReferenceEquals(right, null);
    }
    return left.Equals(right);
}


Avoid branching code for degenerate cases in Fibonacci generator I was writing a simple function that would return the Fibonacci sequence up to the nth term and for whatever reason started wasting a lot of time on it. Here's what I came up with:

static IEnumerable<int> Fibonacci(int n)
{
    int last = 0,
        oneBefore = 0;

    if (n > 0) yield return oneBefore = 0;
    if (n > 1) yield return last = 1;
    if (n > 2)
    {
        while (n-- - 2 > 0)
        {
            yield return last = oneBefore + (oneBefore = last);
        }
    }
}

This is pretty close to what I want, but it has those three ugly if branches at the top level. Initially, I was trying to do something like this for the 0 and 1 case:

yield return (new[] { 0, 1 }).Take(n);

So that it would nicely handle cases where n was 0, 1 or greater. Unfortunately C# doesn't allow mixing of returning complete enumerables with yield statements, so that didn't work.

So here are my questions for what I want to improve:

Is there a way I can avoid the if branches there and somehow elegantly and concisely deal with both degenerate cases?
Is there a way I can avoid having that variable initialization statement at the beginning of the function, or even better, avoid having to keep the two variables at all?

Consider breaking it up into two methods: one, Fibonacci(), that represents the infinite* Fibonacci sequence, and another, Fibonacci(int), that just returns Fibonacci().Take(n). This removes the branching.

An implementation might look like this:

static IEnumerable<int> Fibonacci(int n)
{
    return Fibonacci().Take(n);
}

static IEnumerable<int> Fibonacci()
{
    int a = 0;
    int b = 1;
    while (true)
    {
        yield return a;
        int c = a + b;
        a = b;
        b = c;
    }
}

* Well, the first 47 values that fit in an int.


Compare two lists to see if one is a rotation of the other Going through a list of common interview question, this was the next one. Any suggestions on performance and improvement? What would be a better implementation of this? Also correct me if I'm wrong, this would be \$O(n)\$ complexity?

'''
Problem statement: Given 2 integer arrays, determine if the 2nd array is a     rotated version of the 1st array.
Ex. Original Array A={1,2,3,5,6,7,8} Rotated Array B={5,6,7,8,1,2,3}

@author: Anonymous3.1415
'''

#@cmp_arr, is the array being compared to see if it is a rotation
def is_rotated(int_arr, cmp_arr):

    #lengths of arrays are the same and not equal to zero?
    if len(int_arr) != len(cmp_arr) or len(int_arr) == 0:
        return False

    #does array contain same numbers?
    if not set(int_arr) <= set(cmp_arr) and not set(int_arr) >= set(cmp_arr):
        return False

    #Find start of first list in the second
    index = 0
    while int_arr[0] != cmp_arr[index]:
        index += 1

    #split list at index point, compare the lists
    cmp_arr = cmp_arr[index:] + cmp_arr[:index]
    index = 0
    for x in int_arr:
        if x != cmp_arr[index]:
            return False
        index += 1

    return True

int_arr = [1,2,3,4,6,4,7]
cmp_arr = [6,4,7,1,2,3,4]
print(is_rotated(int_arr, cmp_arr))

Naming

Your 2 arrays are used the same way, it is a bit weird to have 2 different names for them. Maybe int_arr1 and int_arr2 would be better names. Also the fact that they contain integers may be irrelevant for the naming: arr1 and arr2 would do the trick (or lst1 and lst2 because the corresponding Python primitive data structure is list).

Problem with the check for length equal to 0

Checking for identical lengths as the beginning of the function is a nice touch.

However, the or len(lst1) == 0 condition makes me uneasy. It only makes a different when the first part of the check was false and the second part is true which means: len(lst1) == len(lst2) == 0 which really means: lst1 == lst2 == [].

I'd expect the empty array to be a rotation of the empty array. The alternative would be to document the function to precise that only non-trivial rotations are considered (which is not the case at the moment). The easy fix is probably to remove this test or to return True in that case (or whenver lst1 == lst2).

Comments

Commenting your code is nice but adding obvious comments doesn't help anyone. There is a fine line between too many comments and not enough comments.

I'd get rid of #lengths of arrays are the same and not equal to zero? for instance.

On the other hand, #@cmp_arr, is the array being compared to see if it is a rotation could be kept somehow. This should be moved in the function docstring. Also, Python has a Docstring convention called PEP 257. You could write:

def is_rotated(lst1, lst2):
    """Test whether list lst1 is a rotation of list lst2."""

Same value computed multiple times

You compute set(lst1) multiple times which is not as efficient as it could be (which is sad for something meant to be an optimisation).

#does array contain same numbers?
s1 = set(lst1)
s2 = set(lst2)
if not s1 <= s2 and not s1 >= s2:
    return False

Optimisation does not work

I guess the point of the optimisation would be to detect situations like:

lst1, lst2 = [1,2,3,4,6,4,6], [6,4,7,1,2,3,4]
print(is_rotated(lst1, lst2))

lst1, lst2 = [1,2,3,4,6,4,7], [6,4,6,1,2,3,4]
print(is_rotated(lst1, lst2))

Unfortunately, this is not detected.

I guess instead of not s1 <= s2 and not s1 >= s2, you meant: not s1 <= s2 or not s1 >= s2 which is equivalent to not (s1 <= s2 and s1 >= s2) which is equivalent to the more straight-forward: s1 != s2.

In any case, as pointed out by Oscar Smith, this optimisation may not be worth doing.

Reusing existing functions

Looking for index can be done with the already existing function: index = lst2.index(lst1[0]).

Loop like a native

Your final loop is not really Pythonic. Most of the time in Python, you can avoid messing with indices. I hightly recommand Ned Batchelder's talk: "Loop like a native".

In your case, we can improve your code in multiple steps. (Some of the steps are pretty artificial in your case but I use them to show you the different functions you have in your Python toolbox).

Introducing enumerate to get rid of the code to track index:

#split list at index point, compare the lists
rot2 = lst2[index:] + lst2[:index]
for i, x in enumerate(lst1):
    if x != rot2[i]:
        return False
return True

Using zip to avoid accessing rot2 by index:

#split list at index point, compare the lists
rot2 = lst2[index:] + lst2[:index]
for x1, x2 in zip(lst1, rot2):
    if x1 != x2:
        return False
return True

Using all to make things more concise:

return all(x1 == x2 for x1, x2 in zip(lst1, rot2))

The final revelation: all this is not needed:

return lst1 == rot2

At this point, the code looks like:

def is_rotated(lst1, lst2):
    """Test whether list lst1 is a rotation of list lst2."""
    if len(lst1) != len(lst2):
        return False

    if lst1 == lst2:
        return True

    if set(lst1) != set(lst2):
        return False

    index = lst2.index(lst1[0])

    #split list at index point, compare the lists
    rot2 = lst2[index:] + lst2[:index]
    return lst1 == rot2

Broken code

As pointed out by Oscar Smith, your code does not work.

For that kind of programming task, it is very easy to write tests to help you catch issues. These can be written before, during and after writing the code. You could use a proper testing framework or just simple assertions like:

# rotation
lst1, lst2 = [1,2,3,4,6,4,7], [6,4,7,1,2,3,4]
assert is_rotated(lst1, lst2)

# rotation with repeated numbers
lst1, lst2 = [1,2,3,4,6,4,7,1], [6,4,7,1,1,2,3,4]
assert is_rotated(lst1, lst2)

# different set
lst1, lst2 = [1,2,3,4,6,4,6], [6,4,7,1,2,3,4]
assert not is_rotated(lst1, lst2)
lst1, lst2 = [1,2,3,4,6,4,7], [6,4,6,1,2,3,4]
assert not is_rotated(lst1, lst2)

# equal
lst2 = lst1
assert is_rotated(lst1, lst2)

# empty
lst1, lst2 = [], []
assert is_rotated(lst1, lst2)

# 1 empty, 1 not empty
lst1, lst2 = [], [1]
assert not is_rotated(lst1, lst2)
lst1, lst2 = [1], []
assert not is_rotated(lst1, lst2)


Pretty printing degrees input to radians I'm mostly concerned about edge cases, although I think I dealt with them all. Improvements on code quality would also be much appreciated.

/*
    printRadian(90) -> π/2
    printRadian(-90) -> -π/2
    printRadian(180) -> π
    printRadian(-180) -> -π
    printRadian(270) -> 3π/2
*/

function printRadian(degrees) {

    if(degrees % 180 == 0) {
        if(degrees == 180) {
            return "π";
        }
        else if(degrees == -180){
            return "-π";
        }else{
            return degrees / 180 + "π";
        }
    }

    var frac = degrees / 180;

    // 10 ** number of decimal places
    var multiplier = Math.pow(10, frac.toString().split(".")[1].length);
    var numerator = frac * multiplier;
    var denominator = multiplier;

    //http://stackoverflow.com/a/17445304/896112 - Euclid's Algorithm
    function gcd(a, b) {
        return (!b)?a:gcd(b, a%b);
    }

    var g = gcd(numerator, denominator);

    numerator = numerator/g;
    denominator = denominator/g;

    if(numerator == 1){
        numerator = "π";
    }else if(numerator == -1){
        numerator = "-π";
    }else{
        numerator += "π"
    }

    if(denominator < 0){
        denominator = Math.abs(denominator);
        numerator = "-"+numerator;
    }

    return numerator +"/"+ denominator;
}

Your code looks more complicated than necessary and some of your special cases are really not needed.

I will use an incremental development to avoid unnecessary code:

The first version is just based on the formula, with no additional prettiness:

function printRadians(angle) {
  return angle + "π" + "/" + 180;
}

document.write([45, 90, 180, 270, 360, -45, -18].map(printRadians).join('  '));

Then we want to simplify the fraction, using, as you did, the gcd function:

function gcd(a, b) {
  return (b == 0)?a:gcd(b, a%b);
}

function printRadians(angle) {
  var _gcd = gcd(angle, 180)
  return angle / _gcd + "π" + "/" + 180 / _gcd;
}

document.write([45, 90, 180, 270, 360, -45, -18].map(printRadians).join('  '));

We are getting nearer, now we should take care of removing unnecessary ones:

function gcd(a, b) {
  return (b == 0)?a:gcd(b, a%b);
}

function printRadians(angle) {
  var _gcd = gcd(angle, 180)
  var numerator = angle / _gcd == 1 ? "" : angle / _gcd;
  var denominator = 180 / _gcd == 1 ? "" : "/" + (180 / _gcd);
  return numerator + "π" + denominator;
}

document.write([45, 90, 180, 270, 360, -45, -18].map(printRadians).join('  '));

And now let's put the minus in front of the numerator, not denominator:

function gcd(a, b) {
  return (b == 0)?a:gcd(b, a%b);
}

function printRadians(angle) {
  var _gcd = gcd(angle, 180);
  var numerator = angle / _gcd == 1 ? "" : angle / _gcd;
  var denominator = 180 / _gcd == 1 ? "" : "/" + Math.abs(180 / _gcd);
  return ((angle < 0 ? "-" : "") + numerator) + "π" + denominator;
}

document.write([45, 90, 180, 270, 360, -45, -18].map(printRadians).join('  '));

And we are done, just \$4\$ lines, with no lengthy ifs for special cases.


Write a function to determine whether an array contains consecutive numbers for at least N numbers I am trying to write a function to determine whether an array contains consecutive numbers for at least N numbers. For example, the input is [1,5,3,4] and 3, it turns true because the array has 3 consecutive numbers, which is [3,4,5]

Here this function requires sorting beforehand and it is not the most eloquent solution in my opinion. Can someone take a look and suggest some improvements on this?

function hasConsecutiveNums(array, N) {
  if (array.length < N) return false;
  if (N === 0) return true;
  const sortedArray = array.slice().sort((a, b) => a - b);
  let count = 0;
  let prev = null;
  for (const num of sortedArray) {
    if (prev && num === prev + 1) {
      count++;
    } else {
      count = 1;
    }
    if (count === N) return true;
    prev = num;
  }

  return false;
}

console.log(hasConsecutiveNums([1, 4, 5, 6], 3)) // true
console.log(hasConsecutiveNums([1, 4, 5, 6], 4)) // false

One issue to consider: what if an element in the array is 0, and thus falsey? Then if (prev && will not be fulfilled:

console.log(hasConsecutiveNums([-1, 0, 1], 3)) // false... oops

function hasConsecutiveNums(array, N) {
  if (array.length < N) return false;
  if (N === 0) return true;
  const sortedArray = array.slice().sort((a, b) => a - b);
  let count = 0;
  let prev = null;
  for (const num of sortedArray) {
    if (prev && num === prev + 1) {
      count++;
    } else {
      count = 1;
    }
    if (count === N) return true;
    prev = num;
  }

  return false;
}

console.log(hasConsecutiveNums([-1, 0, 1], 3)) // false... oops

Another tweak to make the code a bit more elegant would be to assign prev to the first element of the array first, and initialize count to 1, thus starting comparison on the second element rather than on the first, avoiding the need to compare against null.

With this method, you also need to return true immediately if the array's length is only 1, like the other answer recommends, otherwise there won't be any iterations within which return true could be reached:

function hasConsecutiveNums(array, N) {
  if (array.length < N) return false;
  if (N <= 1) return true;
  const sortedArray = array.slice().sort((a, b) => a - b);
  let prev = sortedArray.shift();
  let count = 1; // first element of the array is already in prev
  for (const num of sortedArray) {
    if (num === prev + 1) {
      count++;
    } else {
      count = 1;
    }
    if (count === N) return true;
    prev = num;
  }

  return false;
}

console.log(hasConsecutiveNums([1, 4, 5, 6], 3)) // true
console.log(hasConsecutiveNums([1, 4, 5, 6], 4)) // false
console.log(hasConsecutiveNums([-1, 0, 1], 3)) // true

If, as the comment notes, you'd want [1, 2, 2, 3] to return true, de-duplicate the numbers with a Set:

function hasConsecutiveNums(array, N) {
  if (array.length < N) return false;
  if (N <= 1) return true;
  const sortedArray = [...new Set(array.slice().sort((a, b) => a - b))];
  let prev = sortedArray.shift();
  let count = 1; // first element of the array is already in prev
  for (const num of sortedArray) {
    if (num === prev + 1) {
      count++;
    } else {
      count = 1;
    }
    if (count === N) return true;
    prev = num;
  }

  return false;
}

console.log(hasConsecutiveNums([1, 4, 5, 6], 3)) // true
console.log(hasConsecutiveNums([1, 4, 5, 6], 4)) // false
console.log(hasConsecutiveNums([-1, 0, 1], 3)) // true
console.log(hasConsecutiveNums([1, 2, 2, 3], 3)) // true