I can has(kell) moar cheezburgers? KITTEH HAS IMPROOVD CODE SKILLS, AN NAO HAS BETTR CODE 4 U

This is an improvement of my original Haskell Lolcats Translator, and now features a function to do the translation (recursively?) using a map. I still have two let statements, which I know are frowned upon, but I couldn't get both the strToUpper and inline fmap working, so I opted for strToUpper.

Sample IO:

Enter your text to translate:
Let me have some cake on a plate please
LEMME HAS SUM CAEK ON PLAET PLZ

Teh Codez

import Data.Map
import Data.Strings

type Dictionary = [(String, String)]
lolcatDictionary =
    [
        (" A ", " "),
        ("CAN I", "I CAN"),
        ("MORE", "MOAR"),
        ("CHEESEBURGER", "CHEEZBURGER"),
        ("HAVE", "HAS"),
        ("HI", "HAI"),
        ("GHOST", "GOAST"),
        ("FEET", "FEAT"),
        ("MOAN", "MOWN"),
        ("CROWD", "CROUD"),
        ("NOTHING", "NUTHING"),
        ("MY", "MAI"),
        ("THEM", "DEM"),
        ("THESE", "THEES"),
        ("YOU", "YU"),
        ("LET ME", "LEMME"),
        ("KITE", "KIET"),
        ("CAT", "KAT"),
        ("CATS", "KATS"),
        ("KITTEN", "KITTEH"),
        ("KITTY", "KITTEH"),
        ("KITTENS", "KITTEHS"),
        ("LIKE", "LIEK"),
        ("COME", "COEM"),
        ("CAME", "CAEM"),
        ("BAKE", "BAEK"),
        ("PLATE", "PLAET"),
        ("SOME", "SUM"),
        ("CAKE", "CAEK"),
        ("PLEASE", "PLZ")
    ]

main :: IO ()
main = do
    putStrLn "Enter your text to translate:"
    input <- getLine
    let intermediate = strToUpper input
    let output = stringMapReplace lolcatDictionary intermediate
    putStrLn output

stringMapReplace :: Dictionary -> String -> String
stringMapReplace [] string = string
stringMapReplace (m:ms) s = stringMapReplace ms (strReplace (fst m) (snd m) s)

What is the Data.Map doing there?  It looks like you were going to use it, but then changed your mind and used a list of pairs instead.

The stringMapReplace function is doing explicit recursion.  A more advanced Haskeller would recognize this pattern as some kind of fold:

-- if the list is empty, the result is the initial value z; else
-- apply f to the first element and the result of folding the rest
foldr f z []     = z
foldr f z (x:xs) = f x (foldr f z xs)

-- if the list is empty, the result is the initial value; else
-- we recurse immediately, making the new initial value the result
-- of combining the old initial value with the first element.
foldl f z []     = z
foldl f z (x:xs) = foldl f (f z x) xs

In this case, the function can be tersely written as

stringMapReplace :: Dictionary -> String -> String
stringMapReplace = flip $ foldr $ uncurry strReplace

Intermediate steps to get there

First, recognize that your dictionary is the thing that is being iterated through.  (That is, your (m:ms) plays the role of the (x:xs) in the fold.)  In Haskell, it is usually more convenient to place the parameter that varies the most at the end.  So, rewrite your original

stringMapReplace (m:ms) s = stringMapReplace ms (strReplace (fst m) (snd m) s)

as

stringMapReplace :: Dictionary -> String -> String
stringMapReplace map s = stringMapReplace' s map
  where
    stringMapReplace' s [] = s
    stringMapReplace' s (m:ms) = stringMapReplace' (strReplace (fst m) (snd m) s) ms

Now, we can see that stringMapReplace' fits the pattern for foldl.  However, foldr is preferable to foldl.  If we perform the substitutions in a different order…

stringMapReplace :: Dictionary -> String -> String
stringMapReplace map s = stringMapReplace' s map
  where
    stringMapReplace' s [] = s
    stringMapReplace' s (m:ms) = strReplace (fst m) (snd m) (stringMapReplace' s ms)

… we can make stringMapReplace' fit the pattern for foldr.

stringMapReplace map s = stringMapReplace' s map
  where
    stringMapReplace' s map = foldr f s map
    f m = strReplace (fst m) (snd m)

Notice that in stringMapReplace' s map = foldr f s map, the s map is repeated on both sides.  We can write that in point-free style as stringMapReplace' = foldr f.

But what is f?  We can simplify it as uncurry strReplace.

stringMapReplace map s = stringMapReplace' s map
  where
    stringMapReplace' = foldr $ uncurry strReplace

What is the relationship between stringMapReplace and stringMapReplace'?  The parameters are flipped.

stringMapReplace = flip $ foldr $ uncurry strReplace

QED.


Linux background changing daemon in Rust I'm working on a program that updates the background of an x11 desktop at a specified interval. However, it eats large amounts of my CPU just sitting idle. I know this is due to the main loop running faster than I want it to. I've used a couple shameful hacks to quell usage down to about 15% but now I am searching for a long-term solution.

Here is the function that contains the main loop:

pub fn run_mapped(&mut self) {
    info!(self.logger, "Running in mapped mode!");
    let wait_d = Duration::new(0,500);
    let long_wait_d = Duration::new(0,750);
    loop {
        if self.img_dir.1.elapsed() > self.reload_time {
            self.img_dir.0.reload();
            self.img_dir.1 = Instant::now();
        }
        let cd = self.x.get_current_desktop();
        // change background if timeout is reached
        if self.since_timeout.elapsed() > self.timeout {
            self.change_backgrounds();
            let ref mut current_bg = self.image_map[cd];
            self.since_timeout = Instant::now();
            self.x.change_background(current_bg);
        }
        match self.x.next_event() {
                Some(_) => {
                    let ref mut current_bg = self.image_map[cd];
                    self.x.change_background(current_bg);
                },
                None => { sleep(wait_d); }
        }
        sleep(long_wait_d);
    }
}

Not shown, but I have additionally set process priority lower to give more important process priority over this one.

I added the sleep calls to prevent >50% CPU usage but it still consumes ~20% which seems a little excessive for what I want this program to do. I'm looking at some of the CPU usage from other programs I'm running in my DE (candybar) and they're nowhere near as resource-hungry as my program.

All that being said, my two questions are:

Why is this? Meaning, why is my program using so much CPU since it is basically 'sleeping' most of the time (in its current state)?
How can I fix this? The only thing I can think of now is scrapping it and abusing cron jobs to the same effect.

The full code can be found on Github.

since it is basically 'sleeping' most of the time

That's not really the most accurate thing...

impl Duration {
    fn new(secs: u64, nanos: u32) -> Duration
}

Duration's second argument is nanoseconds, thus your "long wait" is 750 nanoseconds. That's 0.75 microseconds / 0.00075 milliseconds / 0.00000075 seconds. For reference, a cycle of a 3GHz computer is 0.3333 nanoseconds. If your code takes zero time, you are waking 1 million times per second!

This is the very definition of a busy wait loop.

So how do you fix it? You need to rearchitect. You state:

  and abusing cron jobs to the same effect

Cron jobs have resolution of a minute (8×10^7 bigger!). If you can live with that, sleep for that long.

Another solution is to move to an event-driven system. You make some mention:

  it needs to monitor _NET_CURRENT_DESKTOP which could change at anytime.

If that's the case, you'd have two sources of events: a timer and whatever provides the _NET_CURRENT_DESKTOP event. The current front-runners of this type of code in Rust are futures and mio. I don't know exactly how the X code works, but it's probably event driven (GUI code usually is), and there's probably some mio/futures periodic timer. You "simply" need to configure both of them and then respond whenever either triggers.


Basic Database Class with PDO functionality Is my following (very basic) class considered good use for PDO integration?
Or is there a better, more efficient way?

class Page {

    public $title;
    public $db;

    public function __construct($title) {
        $this->title = $title;
    }

    public function connect($host, $dbname, $username, $password) {
        $this->db = new PDO('mysql:host='.$host.';dbname='.$dbname, $username, $password);
    }

    public function disconnect() {
        $this->db = null;
    }

}

Interestingly, my review is going to be all about control.

Not Enough - Public Property

Just like in the real world, public property can be used by anyone.  If your car was public property anyone could walk into your driveway and do whatever they liked with it.  When you woke up you could not expect to $this->car->drive() (What if someone had already taken it and replaced it with null?).  If they wanted to they could place a house brick in your driveway pretending that it is your car (good luck driving that).

Public properties remove the encapsulation of state from an object.  I see virtually no place for them in OO.  Protected and Private properties on the other hand are set through using the public interface (the public methods) of an object.  Importantly the public interface is testable, so you can ensure that you will not end up with a brick in your driveway unless you really want that.

Too Much - Inversion of Control

Your code will benefit from an Inversion of Conrtol (IoC).  You can read about IoC on the web.  There is a good answer on stackoverflow with code here.

With IoC and the protected properties your code becomes:

class Page {

    protected $title;
    protected $db;

    public function __construct($db, $title) {
        $this->db    = $db;
        $this->title = $title;
    }
}

Your code is no longer dependent on the specific database setup to function correctly.  Your page will use any database connection that it is given.  This has big benefits when it comes to unit testing.  See Miško Hevery - How to Think About the “new” Operator with Respect to Unit Testing.

I have another answer that covers similar things with an extra part on injecting interfaces here.


HtmlAgilityPack Helper class Instead of writing HtmlAgilityPack document code over and over, I've decided to create a helper class for HtmlAgilityPack that prevents me that problem and adds a dispose feature for HtmlDocuments.

What can I do better?

public class HtmlParser
{
    private ArrayList Documents { get; set; }

    public HtmlParser()
    {
        Documents = new ArrayList();
    }

    ~HtmlParser()
    {
        Dispose();
    }

    public string ParseNode(string page, string xPath, ParseType type, string attributes = null)
    {
        int index = Documents.Add(new HtmlDocument());
        ((HtmlDocument)Documents[index]).LoadHtml(page);
        HtmlNode node = ((HtmlDocument)Documents[index]).DocumentNode.SelectSingleNode(xPath);
        if (null != node) return type == ParseType.InnerHtml ? node.InnerHtml : type == ParseType.OuterHtml ? node.OuterHtml : type == ParseType.InnerText ? node.InnerText : node.Attributes[attributes].Value;
        return null;
    }

    public HtmlNode GetNode(string page, string xPath)
    {
        int index = Documents.Add(new HtmlDocument());
        ((HtmlDocument)Documents[index]).LoadHtml(page);
        return ((HtmlDocument) Documents[index]).DocumentNode.SelectSingleNode(xPath);
    }

    public HtmlNodeCollection GetNodes(string page, string xPath)
    {
        int index = Documents.Add(new HtmlDocument());
        ((HtmlDocument)Documents[index]).LoadHtml(page);
        return ((HtmlDocument)Documents[index]).DocumentNode.SelectNodes(xPath);
    }

    public void Dispose()
    {
        if (null != Documents)
            Documents.Clear();
        Documents = null;
        GC.Collect();
    }
}

public enum ParseType
{
    InnerHtml,
    OuterHtml,
    InnerText,
    Attribute
}

There are a few things I've noticed.

This line if (null != node) return type == ParseType.InnerHtml ? node.InnerHtml : type == ParseType.OuterHtml ? node.OuterHtml : type == ParseType.InnerText ? node.InnerText : node.Attributes[attributes].Value; is very confusing.  You are mixing and if statement with multiple ternary really clutters up that line of code.
You shoud look into the var keyword.  When the variable type is obvious, it is pretty much stand practise to use it.  I think it really cleans up code.
I'm not sure why you are using GC.Collect() in your constructor.  In C# there is not usually a need to control Garbage Collection.  The framework deals with it pretty well.
I would change if (null != Documents) to if (Documents != null).  I know why that syntax is done in C/C++, but in C# it is not needed, and the latter is much more descriptive and easier to understand.
I would be careful about returning null from a method.  It adds extra code where the method is called to check for null.  Maybe look into the null object design pattern

Other than that, your use of white space and indentation is very well done.  Makes reading your code much easier.


Generic C++ exception catch handler macro I have this set of legacy C++ projects with a large number of public functions.  At the start, none of those publicly exposed functions had try..catch insulation inside them.  When a C++ exception fired across that boundary, if the caller wasn't compiled with the same C++ compiler and the same project settings, then it easily caused a crash.

To help insulate against this problem I first went into every public function and wrapped the public function bodies with all-encompassing try...catch blocks, with the catch attempting to log a message about the exception.  Basically just handling the (...) case and logging "Unexpected Exception".

With hundreds of public functions (working up to thousands), this became tedious when I decided to add specialized std::exception handlers to them all, other dev team members update some to do something different, etc.

In the interests of DRY (Don't Repeat Yourself), I chose to make a generic catch block which gets applied in those instances where I wanted something fast to insulate a function from throwing an exception but get as much info as possible into the log.

C++ exceptions being what they are, I couldn't figure out a nice simple portable way to do it without using a macro.  Yes, yes, sad but true, another macro is born, but I am interested to learn about any other options considering it must be portable and fast.  I don't mean to say macros are portable and fast, but this implementation fits the bill...

#define CatchAll( msg ) \
    catch( const Poco::Exception &e )   \
    {   \
        try{ LogCritical( Logs.System(), std::string( e.displayText() ).append( msg ) );}catch(...){assert(0);} \
    }   \
    catch( const std::exception &e )    \
    {   \
        try{LogCritical( Logs.System(), std::string( e.what() ).append( msg ) );}catch(...){assert(0);} \
    }   \
    catch(...)  \
    {   \
        try{ LogCritical( Logs.System(), std::string( "Exception caught in " __FUNCTION__ ". " ).append( msg ) );}catch(...){assert(0);}    \
    }

So there you have it.  The code above gets called like this...

try{
   // statements that can throw
}
CatchAll("*Special extra message about what was attempted in the try block*")

So that's it, that's the magic.  This isn't meant to replace an intelligently coded block of specific exception handling, this is meant to quickly put bare-bones insulation where none existed before, and do it in a DRY, portable, fast and easy to grok way.

Ok, ok, macros are evil, I know, but what else could be done here?

And as goes the way of the macro, they proliferate and multiply.  Here's a secondary macro to set an rc code in addition to logging, so the function can return a failing rc if an exception throws in the insulated function...

/// Catch all generic exceptions and log appropriately.
/// Logger is insulated from throwing, so this is a NO THROW operation.
/// Sets rc (integer parameter) so wrapping function can perform cleanup
/// before returning the error number.
#define CatchAllSetRC( rc, msg )    \
    catch( const Poco::Exception &e )   \
    {   \
        (rc) = -1;  \
        try{ LogCritical( Logs.System(), std::string( e.displayText() ).append( msg ));}catch(...){assert(0);}  \
    }   \
    catch( const std::exception &e )    \
    {   \
        (rc) = -1;  \
        try{ LogCritical( Logs.System(), std::string( e.what() ).append( msg ));}catch(...){ assert(0); }   \
    }   \
    catch(...)  \
    {   \
        (rc) = -1;  \
        try{ LogCritical( Logs.System(), std::string( "Exception caught in " __FUNCTION__ ". " ).append( msg ));}catch(...){ assert(0); }   \
    }

This expanded version gets called with an rc code so the caller can return it...

int rc = 0;
try{
   // statements that can throw
}
CatchAll(rc, "Extra info to append to logged exception message")

In the catch block, you can rethrow the exception. This can be done in a function:

int rc = 0;
try{
// statements that can throw
}catch(...){
    HandleException(rc, "Extra info to append to logged exception message");
}

void HandleException(int rc, std::string msg)
{
    try{
        throw;
    }catch(Poco::Exception &e){
        // ...
    }catch( const std::exception &e){
        // ...
    }
}


Handling a WordPress form with multiple checkboxes if (isset($_POST["save-options"])) {

    foreach ($names_of_array as $type => $value) {

        update_option('updated_img', $_POST['show_img_url']);

        if (isset($_POST["post0"])) {

            update_option($names_of_array[0], true);
        } else {

            update_option($names_of_array[0], false);
        }

        if (isset($_POST["post1"])) {

            update_option($names_of_array[1], true);
        } else {

            update_option($names_of_array[1], false);
        }

        if (isset($_POST["post2"])) {

            update_option($names_of_array[2], true);
        } else {

            update_option($names_of_array[2], false);
        }

        if (isset($_POST["post3"])) {

            update_option($names_of_array[3], true);
        } else {

            update_option($names_of_array[3], false);
        }
        if (isset($_POST["post4"])) {

            update_option($names_of_array[4], true);
        } else {

            update_option($names_of_array[4], false);
        }
    }
}

HTML table is in form>>>>

                <input name="post0"  value="<?php echo $names_of_array; ?>"  type="checkbox" <?php echo (get_option("show_price") == true ? 'checked' : ''); ?>   >
                <input name="post1" value="<?php echo $names_of_array; ?>"  type="checkbox" <?php echo (get_option("show_width") == true ? 'checked' : ''); ?> >
                <input name="post2" value="<?php echo $names_of_array; ?>"  type="checkbox" <?php echo (get_option("show_height") == true ? 'checked' : ''); ?> >
                <input name="post3" value="<?php echo $names_of_array; ?>"  type="checkbox" <?php echo (get_option("show_weight") == true ? 'checked' : ''); ?> >
                <input name="post4" value="<?php echo $names_of_array; ?>"  type="checkbox" <?php echo (get_option("show_color") == true ? 'checked' : ''); ?> >
                <input name="updated_img"  type="button" id="insert-my-media" class="media-button button"  value="Add my media">
                <input name="show_img_url" type="text" class="next-button" id="set_imaged_id" value="<?php echo $_POST['show_img_url']; ?>" >

You are open to reflected XSS via show_img_url. You should always HTML encode user input when outputting it.
You don't really need to add the class to each cell. Just add it to the table to avoid duplication and increase readability.
Your spacing and indentation is slightly off (the tr elements don't align, and the first td element has too many spaces)


Computing max t such that tanh(pi*sinh(t)/2) <1 I'm looking for a way to quickly determine that maximum value of t such that tanh(pi/2 sinh(t)) is strictly less than 1, for a given precision.

I have reproduced the code I am currently using to do this below:

#include <cmath>
#include <iostream>
#include <random>
#include <boost/math/special_functions/next.hpp>
#include <boost/math/constants/constants.hpp>
#include <boost/multiprecision/float128.hpp>
#include <boost/multiprecision/cpp_bin_float.hpp>
#include <benchmark/benchmark.h>

using std::tanh;
using std::sinh;
using std::asinh;
using std::atanh;
using std::isnormal;
using boost::math::float_prior;
using boost::math::constants::half_pi;
using boost::math::constants::two_div_pi;
using boost::multiprecision::float128;
using boost::multiprecision::cpp_bin_float_50;
using boost::multiprecision::cpp_bin_float_100;

template<class Real>
Real g(Real t)
{
    Real tmp = half_pi<Real>()*sinh(t);
    return tanh(tmp);
}

template<class Real>
Real g_inv(Real x)
{
    return asinh(two_div_pi<Real>()*atanh(x));
}

template<class Real>
Real find_max_t()
{
    Real x = float_prior((Real) 1);
    Real t_max = g_inv(x);
    while(!isnormal(t_max))
    {
        x = float_prior(x);
        t_max = g_inv(x);
    }
    // This occurs once on 100 digit arithmetic:
    while(!(g(t_max) < (Real) 1))
    {
        x = float_prior(x);
        t_max = g_inv(x);
    }
    return t_max;
}

template<class Real>
static void BM_max_t(benchmark::State& state)
{
    Real t_max;
    while(state.KeepRunning())
    {
        benchmark::DoNotOptimize(t_max = find_max_t<Real>());
    }
    std::cout << "t_max = " << t_max << std::endl;
}

BENCHMARK_TEMPLATE(BM_max_t, float);
BENCHMARK_TEMPLATE(BM_max_t, double);
BENCHMARK_TEMPLATE(BM_max_t, long double);
BENCHMARK_TEMPLATE(BM_max_t, float128);
BENCHMARK_TEMPLATE(BM_max_t, cpp_bin_float_50);
BENCHMARK_TEMPLATE(BM_max_t, cpp_bin_float_100);

BENCHMARK_MAIN();

The build sequence is:

INC = -I/path/to/boost
all: perf.x

perf.x: perf.o
    g++ -o $@ $< -lbenchmark -pthread -lquadmath

perf.o: perf.cpp
    g++ --std=c++14 -fext-numeric-literals -Wfatal-errors -g -O3 $(INC) -c $< -o $@

This computes the correct value of t_max in 153 ns (float), 214 ns (double), 246 ns (long double),  5280 ns (float128), 53us (cpp_bin_float_50), and 455 us (cpp_bin_float_100).

How can this code be made faster? For reference, at double precision, I have found 30% of the time is spent calling __expm1, 24% calling __ieee754_log_avx, and 15% calling __asinh.

I don't have any suggestions for making the code faster (other than pre-computing at compile-time, which I assume isn't an option for you), but I do have some general observations on the code style.

Instead of bringing definitions of the std and boost names at global scope, they can be isolated in the functions that use them (still with using, so ADL will continue to work the same as in the original code):

#include <cmath>
#include <boost/math/constants/constants.hpp>

template<class Real>
Real g(Real t)
{
    using std::tanh;
    using std::sinh;
    using boost::math::constants::half_pi;
    Real tmp = half_pi<Real>()*sinh(t);
    return tanh(tmp);
}

template<class Real>
Real g_inv(Real x)
{
    using std::asinh;
    using std::atanh;
    using boost::math::constants::two_div_pi;
    return asinh(two_div_pi<Real>()*atanh(x));
}

Standard C++ provides nexttoward(), which can be used in place of boost::math::float_prior:

template<class Real>
Real find_max_t()
{
    using std::isnormal;
    using std::nexttoward;

    static const Real ONE{1};

    Real x = nexttoward(ONE, 0);
    Real t_max = g_inv(x);
    while (!isnormal(t_max))
    {
        x = float_prior(x);
        t_max = g_inv(x);
    }
    // This occurs once on 100 digit arithmetic:
    while (!(g(t_max) < ONE))
    {
        x = nexttoward(x, 0);
        t_max = g_inv(x);
    }
    return t_max;
}

Unfortunately, the Boost numeric types don't provide any nexttoward(), so you may be stuck using boost::math::float_prior after all.  :-(

The benchmark code can now be separable:

#include <iostream>
#include <benchmark/benchmark.h>

template<class Real>
static void BM_max_t(benchmark::State& state)
{
    Real t_max;
    while(state.KeepRunning())
    {
        benchmark::DoNotOptimize(t_max = find_max_t<Real>());
    }
    std::cout << "t_max = " << t_max << std::endl;
}

#include <boost/multiprecision/float128.hpp>
#include <boost/multiprecision/cpp_bin_float.hpp>
BENCHMARK_TEMPLATE(BM_max_t, float);
BENCHMARK_TEMPLATE(BM_max_t, double);
BENCHMARK_TEMPLATE(BM_max_t, long double);
BENCHMARK_TEMPLATE(BM_max_t, boost::multiprecision::float128);
BENCHMARK_TEMPLATE(BM_max_t, boost::multiprecision::cpp_bin_float_50);
BENCHMARK_TEMPLATE(BM_max_t, boost::multiprecision::cpp_bin_float_100);

BENCHMARK_MAIN();


Storing three integer values Consider an 8-bit complier, in which we have an integer of size, say
  2-words i.e. 16-bits or 2 bytes. You need to store three integer
  values which are supposed to be as:

Name        Range       Size Required
--------------------------------------------------
Integer 1    0-3        2bits (00-11   in binary)
Integer 2    0-5        3bits (000-101 in binary)
Inetger 3    0-7        3bits (000-111 in binary)

  In total, we need only 8 bits to store all of these three values.
  Which means that a single variable of integer datatype has enough
  memory to accommodate these values. The challenge is to store all of
  these three variables into a single instance of integer datatype and
  retrieve it whenever required.

  In other words, you need to develop a program which can - at the
  maximum - use only two instances of integer datatype, first one for
  the purpose of scanning values from the standard input device say
  keyboard and the second one to store the data entered by user in the
  form of three integer variables as discussed above and print them by
  retrieving it from the second instance in which the data was stored.

This is my program with no input validation.  Please review it.

#include <stdio.h>
#include <assert.h>

#ifdef DEBUG
void test();
#endif

#define store(_out, _in, _pos) ((_out) |= ((_in) << (_pos)))
#define ret(_out, _pos, _mask) (((_out) >> (_pos)) & (_mask))

#define firstnum_pos  0
#define secondnum_pos 2
#define thirdnum_pos  5

#define firstnum_mask  0x3
#define secondnum_mask 0x7
#define thirdnum_mask  0x7

int main()
{
  unsigned short input = 0, output = 0;
  printf ("Enter first number:");
  scanf ("%hu", &input);
  store(output, input, firstnum_pos);
  printf ("Enter second number:");
  scanf ("%hu", &input);
  store(output, input, secondnum_pos);
  printf ("Enter third number:");
  scanf ("%hu", &input);
  store(output, input, thirdnum_pos);

  printf ("first  number: %d", ret(output, firstnum_pos, firstnum_mask));
  printf ("second number: %d", ret(output, secondnum_pos, secondnum_mask));
  printf ("third  number: %d", ret(output, thirdnum_pos, thirdnum_mask));

#ifdef DEBUG
  test();
#endif

  return 0;
}

#ifdef DEBUG
void test()
{
  int i,j,k;
  short out;
  for (i=0; i<=3; i++)
    for (j=0; j<=5; j++)
      for (k=0; k<7; k++)
        {
          out = 0;
          store(out, i, firstnum_pos);
          store(out, j, secondnum_pos);
          store(out, k, thirdnum_pos);
          assert (i == ret(out, firstnum_pos, firstnum_mask));
          assert (j == ret(out, secondnum_pos, secondnum_mask));
          assert (k == ret(out, thirdnum_pos, thirdnum_mask));
        }
  printf ("Test completed\n");
}
#endif

Some ideas:

I'd try encapsulating the position and mask parameters to one parameter. Maybe a struct would be fine for this. It is easy to mix up the parameters of ret, for example, somebody could call it accidentally with

`ret(output, secondnum_pos, firstnum_mask)`

or with

`ret(output, firstnum_mask, firstnum_pos)`.

(Note the parameter order.)
Consider writing tests which store the i, j, k values in different order. Currently they do not detect if storing at the firstnum_pos overrides the second or the third values.
Consider making copies of i, j and k for the assert calls before you call the marcos. If a macro accidentally changes values of i, j or k the assert will not detect it.
A unit testing framework would be better for the testing: Unit Testing C Code


Input text sizing function I have this jQuery function I wrote up to set the size of an input based off of its data. In this case I have already figured out the widths for each character based off of a 12px font size for Helvetica.

It's not super flexible or anything, but I would like to know if there is any good way to take what I have and maybe make it more elegant (or fewer lines of code).

HTML:

<input value="Some really long string about nothing important"" />

CSS:

body {
    font-family:Helvetica;
    font-size:12px;
}
span {
    padding:0;
    margin:0;
}
input {
    border:0;
}

JavaScript / jQuery:

// Helvetica, 12px or 1.2em at 62.5%
var str = $('input').val();
var length = str.length;

var charWidth = 0;
for ( var i = 0; i < length; i++ )
{
    console.log(str[i]);

    if ( str[i] == "f" ||
         str[i] == "i" ||
         str[i] == "I" ||
         str[i] == "j" ||
         str[i] == "l" ||
         str[i] == "t"
       )
    {
        charWidth += 3;
    }
    if ( str[i] == "r" )
    {
        charWidth += 4;
    }
    if ( str[i] == "v" ||
         str[i] == "x" ||
         str[i] == "y" ||
         str[i] == "z"
       )
    {
        charWidth += 5;
    }
    if ( str[i] == "c" ||
         str[i] == "k" ||
         str[i] == " "
       )
    {
       charWidth += 6;
    }
    if ( str[i] == "a" ||
         str[i] == "A" ||
         str[i] == "b" ||
         str[i] == "d" ||
         str[i] == "e" ||
         str[i] == "F" ||
         str[i] == "g" ||
         str[i] == "h" ||
         str[i] == "J" ||
         str[i] == "L" ||
         str[i] == "n" ||
         str[i] == "o" ||
         str[i] == "p" ||
         str[i] == "q" ||
         str[i] == "s" ||
         str[i] == "T" ||
         str[i] == "u" ||
         str[i] == "V" ||
         str[i] == "X" ||
         str[i] == "Y" ||
         str[i] == "Z" ||
         str[i] == "0" ||
         str[i] == "1" ||
         str[i] == "2" ||
         str[i] == "3" ||
         str[i] == "4" ||
         str[i] == "5" ||
         str[i] == "6" ||
         str[i] == "7" ||
         str[i] == "8" ||
         str[i] == "9"
       )
    {
        charWidth += 7;
    }
    if ( str[i] == "B" ||
         str[i] == "E" ||
         str[i] == "K" ||
         str[i] == "P" ||
         str[i] == "S"
       )
    {
        charWidth += 8;
    }
    if ( str[i] == "C" ||
         str[i] == "D" ||
         str[i] == "G" ||
         str[i] == "H" ||
         str[i] == "M" ||
         str[i] == "N" ||
         str[i] == "O" ||
         str[i] == "Q" ||
         str[i] == "R" ||
         str[i] == "U" ||
         str[i] == "w"
       )
    {
        charWidth += 9;
    }

    if ( str[i] == "m" || str == "W")
    {
        charWidth += 11;
    }
}

$('input').css({'width': charWidth});

jsFiddle

Here's how you could use a table array for the widths.  I didn't fill in all the character widths (I just did lowercase) because it's tedious to build, but you can extend it to contain all the typeable characters that you want to support.  I also made it a jQuery method that you can call like this:

$("input").autoSize();

Here's the code (also in a jsFiddle):

(function() {
    var widths = [
     // a, b, c, d, e, f, g
        7, 7, 6, 7, 7, 3, 7,
     // h, i, j, k, l, m, n
        7, 3, 3, 6, 3, 11, 7,
     // o, p, q, r, s, t, u
        7, 7, 7, 4, 7, 3, 7,
     // v, w, x, y, z
        5, 9, 5, 5, 5
    ];

    // character code our table starts with
    var lowWidth = 97;

    $.fn.autoSize = function() {
        return this.each(function() {
            var val = this.value;
            var totalWidth = 0, charIndex, ch;
            for (var i = 0, len = val.length; i < len; i++) {
                // get char code and see if it's in our width table
                charIndex = val.charCodeAt(i) - lowWidth;
                ch = val.charAt(i);
                if (charIndex >= 0 && charIndex < widths.length) {
                    totalWidth += widths[charIndex];
                } else if (ch == ' ') {
                    // special case for space char
                    // until the table contains all codes we need
                    totalWidth += 6;
                }
            }
            $(this).css("width", totalWidth + "px");
        });
    }
})();

FYI, jQuery will also measure the natural width of any DOM element for you so you could also have a span that is styled with the right font that you insert this text into and then ask jQuery what it's width is.  jQuery will temporarily make it position absolute (so it will layout to it's natural width) and then let the browser tell you how wide it is.  This would be much, much more accurate than what you are doing.

Letting jQuery measure it for you would look like this:

$.fn.autoSize = function() {
    var testItem = $("#testWidth");
    return this.each(function() {
        // put the text into our test span
        testItem.text(this.value);
        $(this).width(testItem.width());
    });
}

And, you'd have appropriate CSS for a testItem span (see the jsFiddle demo for details):

jsFiddle demo: http://jsfiddle.net/jfriend00/jQ93f/


Drag and normal upload code This is a partial code of my JavaScript app.

The openDoc function's task is:
call newDoc if use uploads a file through fileInput.
Or, when user drag and drop a file in the document.

I want to review my logic and code.

      function stopDefault(event) {
        event.stopPropagation();
        event.preventDefault();
      }

      function openDoc(event) {
        var files = event.target.files || event.dataTransfer.files,
          file = files[0],
          reader = new FileReader();
        if (file) { // prevent TypeError
          stopDefault(event); // prevent default file drop behavior
          reader.readAsText(file);
        }
        reader.addEventListener("load", function(event) {
          newDoc(event.target.result, file.name);
        });
      }

  fileInput.addEventListener("change", openDoc);
  document.addEventListener("dragover", stopDefault);
  document.addEventListener("drop", openDoc);

If you're interested, checkout the full code - https://dl.dropboxusercontent.com/u/92126558/app.js

There is a not a lot to review, from a once over:

Indenting is not good for stopDefault, but I guess that's from copy pasting
There is no sense in calling addEventListener if file is not set, it should be part of your if block
Personally I would first add the listener, then starting reading text
I like your use of stopDefault
JsHint could find nothing wrong
Looks easy to maintain and to grok


How to optimize Method Visibility and code structures for better performance? I'm creating an application to get data cross domain all the line of code seem work as well but I'm not much understand about performance with my code process with the real server and real action because sometime there are a lot of case will happen when I fetch those data so I want to make a method to get those errors by using getHttpCode method and however fetch and initial data in this httpData method. Moreover I want to know when I go to real server and real real time It will be facing a lot of case so I want to check that cause or errors and send it to view by using s_respond mean that Server respond with HTTP_code.

Please check my below function

    private function httpData($url =null, $id = null)
    {
        if($id){
            $url = 'website/api/v1/pro/'.$id;
        }
        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $url);
        curl_setopt($ch, CURLOPT_FOLLOWLOCATION, 1);
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
        curl_setopt($ch, CURLINFO_PRETRANSFER_TIME, 30);
        curl_setopt($ch, CURLINFO_HTTP_CODE, true);
        curl_setopt($ch, CURLOPT_PRIVATE, true);
        curl_setopt($ch, CURLOPT_TIMEOUT, 30);

        if (!$executed = curl_exec($ch)) {

            $res = $executed;
            $data = false;
            curl_close($ch);
        } else {

            if ($this->http_code = $this->getHttpCode(curl_getinfo($ch))) {

                $res = $this->http_code;
                $data = $executed;
            } else {
                $res = false;
            }
        }
        return ['s_respond' => $res, 'data' => $executed];
    }

    private function getHttpCode($http)
    {
        if (is_array($http)) {
            if (!empty($http['http_code'] || $http['http_code'] != 0)) {
                return $http['http_code'];
            } else {
                return false;
            }
        } else {
            return false;
        }
    }

//Initial and send to view function....

public function products()
    {
        $url = 'http://website/api/v1/products';
        $data = $this->httpData($url);
        return view('products.list', ['data'=>$data]);
    }

Everything looks good enough.

Code style is OK.

You are using curl, so it will be quite fast and there is nothing you can do in the code to speed it up.

You may or may not need to check this:

    if($id){   // (1)
        $url = 'website/api/v1/pro/'.$id; // (2)
    }

depends from where this $id comes, you probably need to sanitize it somehow. see (1)

In same place you overwrite the $url. see (2) You probably want this:

    if($id){
        $url = $url . 'website/api/v1/pro/'.$id;
    }

I also do not see where you do curl_close($ch) in case an error happen.


Implementation of binary tree traversal Based on what i understood of Binary Tree, queue and recursion I implemented this Breadth First Search algorithm as follows.
Do you have any suggestions to improve (in term of readability, good practice etc...) it?
# Definition for a binary tree node.
class Node(object):
    def __init__(self, val=0, left=None, right=None):
        self.val = val
        self.left = left
        self.right = right

    def preorder_print(self):
        print(self.val)
        if self.left:
            self.left.preorder_print()
        if self.right:
            self.right.preorder_print()
        return

    def postorder_print(self):
        if self.left:
            self.left.postorder_print()
        if self.right:
            self.right.postorder_print()
        print(self.val)
        return

    def inorder_print(self):
        if self.left:
            self.left.inorder_print()
        print(self.val)
        if self.right:
            self.right.inorder_print()
        return

    def bread_traversal_print(self):
        q=Queue()
        q.push(self)
        def bt():
            #import pdb;pdb.set_trace
            if not q:
                return
            node=q.pop()
            if node:
                if  node.left:
                    q.push(node.left)
                if  node.right:
                    q.push(node.right)
                print(node.val)
                bt()
        bt()

class Queue():
    def __init__(self):
        self.data = []

    def push(self,elt):
        self.data.append(elt)

    def pop(self):
        if self.data:
            return self.data.pop(0)
    def peek(self):
        if self.data:
            return self.data[0]

if __name__ == "__main__":
    #input = [l for l in 'FDJBEGKACIH']
    root = Node(1)
    root.left = Node(2)
    root.right = Node(3)
    root.left.left = Node(4)
    root.left.right = Node(5)
    root.right.left = Node(6)
    root.right.right = Node(7)

    print("preorder:")
    root.preorder_print()
    print("postorder:")
    root.postorder_print()
    print("inorder:")
    root.inorder_print()

    print('bread :')
    root.bread_traversal_print()

    """          1
            2       3
        4     5    6  7

    [1 2 4 5 3 6 7]

    """

EDIT: Yes BFS for Breadth First Search would be more appropriate ;)

Class definition
There is no need to explicitly inherit from object; that is a Python 2.x anachronism.
Instead of class Node(object):, simply write class Node:.
Similarly, class Queue(): can simply be class Queue:.
Default Parameters
Every Node should have a value; there is little reason to provide a default of 0 for the val parameter.  By forcing the caller to provide the value, you are less likely to accidentally have a node with the default integer 0 value when all the other nodes are created with (say) strings.
Unnecessary returns
There is no need for the return statements in preorder_print, postorder_print, and inorder_print.
Visitor pattern
You have defined several methods for traversing the tree.  All of them print the values.  If you wanted to do anything else with the values, you're stuck writing another (set of 4?) function.
Instead, you should separate the traversal from the operation.  Visit each node, in some order, and do something at that node.
class Node:

    ...

    def preorder(self, visitor):
        visitor(self.val)
        if self.left:
            self.left.preorder(visitor)
        if self.right:
            self.preorder(visitor)

    ...

# Do a pre-order traversal the tree, starting at the root, printing each values.
root.preorder(print)

Breadth First Traversal
First, the function should probably be called breadth_traversal_print, not bread_traversal_print.
Second, there is no need to use recursion during a breadth first traversal.  You have a Queue which keeps track of the nodes you need to visit.  You just need to loop while the queue is not empty.  The recursion is doing a loop in a resource-intensive fashion.  If Python had tail-call-optimization, the inefficiency could be removed automatically; but Python doesn't, so looping this way is wrong.
    def breadth_traversal_print(self):
        q = Queue()
        q.push(self)

        while q:
            node = q.pop()
            if node:
                if node.left:
                    q.push(node.left)
                if node.right:
                    q.push(node.right)
                print(node.val)

PEP-8
The PEP 8 -- Style Guide for Python enumerates many style guidelines all programs should follow.
White Space
There should be a space around binary operators, like =.  You violate this with q=Queue().  Note that = used with keyword arguments is not an operator, so doesn't shouldn't be surrounded by white space; therefore def __init__(self, val=0, left=None, right=None): is correct.
There should be a space after commas.  You violate this in def push(self,elt):
No-op code
This string ...
    """          1
            2       3
        4     5    6  7

    [1 2 4 5 3 6 7]

    """

... is a statement which does nothing, and has no side-effect.  Perhaps you meant this to be a comment?
NOTE: This is different from """docstrings""".  A string statement which immediately follows a class or def statement is treated as a docstring, removed from the program code, and stored in the __doc__ member of the class or function by the Python interpreter.  There, it can be extracted by help(...) and other documentation generation programs.


Finding duplicate invoices I have a invoiceList which is a List<Map<String:String>> and am trying to find out if all the invoices have same SENDER_COUNTRY and CLIENT_COUNTRY or not. If not, it will add the message to a JSON array.

[
    [INVOICE_DATE:20150617, INVOICE_NUMBER:617151,SENDER_COUNTRY:USA, CLIENT_COUNTRY:USA]
    [INVOICE_DATE:20150617, INVOICE_NUMBER:617152,SENDER_COUNTRY:CAD, CLIENT_COUNTRY:MEX]
    [INVOICE_DATE:20150617, INVOICE_NUMBER:617153,SENDER_COUNTRY:CAD, CLIENT_COUNTRY:MEX]
]

JSONArray jsonArray = new JSONArray();
def senderCountry = invoiceList[0]['SENDER_COUNTRY']
def clientCountry  = invoiceList[0]['CLIENT_COUNTRY']
invoiceList.each{ it ->
if(it['SENDER_COUNTRY'] != senderCountry)
  jsonArray.add((new JSONObject()).put("SENDER_COUNTRY","Multiple sender Countries Associated"));
 if(it['CLIENT_COUNTRY'] != clientCountry)
  jsonArray.add((new JSONObject()).put("CLIENT_COUNTRY","Multiple Client Countries Associated"));
}

I feel this code can be refactored/optimized to a better version in Groovy. Can someone please help me with it?

Putting the JSONArray aside (given I don't know the fully qualified name of the class), here's a Groovy-er version:

Working example

def invoiceList = [
    [INVOICE_DATE:20150617, INVOICE_NUMBER:617151,SENDER_COUNTRY:'USA', CLIENT_COUNTRY:'USA'],
    [INVOICE_DATE:20150617, INVOICE_NUMBER:617152,SENDER_COUNTRY:'CAD', CLIENT_COUNTRY:'MEX'],
    [INVOICE_DATE:20150617, INVOICE_NUMBER:617153,SENDER_COUNTRY:'CAD', CLIENT_COUNTRY:'MEX']
]

def senderCountry = invoiceList[0]['SENDER_COUNTRY']
def clientCountry  = invoiceList[0]['CLIENT_COUNTRY']

def result = invoiceList.inject([:]) { status, invoice ->
    if(invoice['SENDER_COUNTRY'] != senderCountry) status << ["SENDER_COUNTRY": "Multiple sender Countries Associated"]
    if(invoice['CLIENT_COUNTRY'] != clientCountry) status << ["CLIENT_COUNTRY":"Multiple Client Countries Associated"]

    status
}

if(result) {
    println "Not all invoices have the same sender and client countries."
} else {
    println "All good"
}

How it works

The inject() method iterates through the invoiceList and calls a closure with an object to aggregate with and the current item in the list. Iteration begins with an empty Map as the aggregated object, which remains empty unless the un-matching countries are found. When a mismatch occurs, it is recorded in the Map. The closure purposely returns the Map because the output of the closure is what's fed back in during the next iteration.

Finally, if the resulting Map is empty, then all of the countries are the same. Otherwise, the Map contains one or two keys indicating what the problem was.

If all you really need to know is whether all the countries matched or not, you can simply use every():

def result = invoiceList.every { invoice ->
    invoice['SENDER_COUNTRY'] == senderCountry && invoice['CLIENT_COUNTRY'] == clientCountry
}


XML Python can this be improved ok spent ages on this simple problem (I'm not good at XML) below code is probably self explanatory and it works, but i think this could be done a lot easier

Sample XML

<data>
        <id name="ch01">
                <channel>test channel 1</channel>
                <gpio>3</gpio>
                <cont>front</cont>
        </id>

        <id name="ch02">
                <channel>test channel 2</channel>
                <gpio>5</gpio>
                <cont>back</cont>
        </id>
</data>

The code used, which I eventually got working after reading a few posts on here:

from xml.dom import minidom
dom = minidom.parse('/usr/local/sbin/logger/chaninfo_test.xml')
id=dom.getElementsByTagName('id')
for node in id:
        chanid=node.getAttribute('name')
        print chanid
        clist=node.getElementsByTagName('channel')
        for c in clist:
                print c.childNodes[0].nodeValue

        glist=node.getElementsByTagName('gpio')
        for g in glist:
                print g.childNodes[0].nodeValue

        colist=node.getElementsByTagName('cont')
        for co in colist:
                print co.childNodes[0].nodeValue
        print

Can this be improved or cleaned up? The XML will increase in size but contents won't change, multiple IDs with one channel, gpis and cont field each.

The first thing that I'm thinking about is combining is and name is channel id='name', etc.

Use for in loops to avoid repetition

These 3 blocks of code are very similar:

1

    clist=node.getElementsByTagName('channel')
    for c in clist:
            print c.childNodes[0].nodeValue

2

    glist=node.getElementsByTagName('gpio')
    for g in glist:
            print g.childNodes[0].nodeValue

3

    colist=node.getElementsByTagName('cont')
    for co in colist:
            print co.childNodes[0].nodeValue

We can clean up by abstracting what is different (the name) into a loop variable:

for name in ('channel', 'gpio', 'cont'):
    for element in node.getElementsByTagName(name):
        print element.childNodes[0].nodeValue

Now also adding another name will lead to only adding it to the list instead of repeating similar code the fourth time.


Checking if blocks are catch extendable Basically, I have a list, newtry of blocks, and I want to find a value of catchtype that makes them all return true for block.isCatchExtendable(newhandler, catchtype) or report an error if this can't be done. block.isCatchExtendable returns two values, success and a new type to try on failure.

My current code works, but the control flow is very convoluted, with numerous breaks and elses. I was wondering if anyone could think of a better way to arrange things. Also, these are all my own functions, so I am free to change the interfaces, etc.

while 1:
    ct_changed = False

    for block in newtry:
        success, newcatchtype = block.isCatchExtendable(newhandler, catchtype)
        if not success:
            if catchtype == newcatchtype: #no success and no other types to try, so just break and fail
                break
            catchtype, ct_changed = newcatchtype, True
    else:
        if ct_changed:
            continue
        else:
            break
    error('Unable to extend try block completely')

isCatchExtendible:

def isCatchExtendable(self, newhandler, catchtype):
    return self.catchset.extendible(newhandler, catchtype, self.currentHandlers)

This then calls:

#If not extendible with current catchtype, returns suggested type as second arg
def extendible(self, newhandler, catchtype, outerhs):
    if catchtype is None:
        temp = self.catchsets.get(newhandler)
        if temp is None:
            return True, None
        return False, temp.getSingleTType()[0]

    proposed = ExceptionSet.fromTops(self.env, catchtype)
    inner = [h for h in self.handlers if h != newhandler and h not in outerhs]
    outer = [h for h in self.handlers if h in outerhs]

    sofar = ExceptionSet.fromTops(self.env)
    for h in inner:
        sofar = sofar | self.catchsets[h]
    if (proposed - sofar) != self.catchsets[newhandler]:

        #Get a suggsted catch type to try instead
        suggested = (self.catchsets[newhandler] | sofar).getSingleTType()
        suggested = objtypes.commonSupertype(self.env, suggested, (catchtype, 0))
        assert(self.env.isSubClass(suggested[0], 'java/lang/Throwable'))
        return False, suggested[0]
    for h in outer:
        if not proposed.isdisjoint(self.catchsets[h]):
            return False, catchtype
    return True, catchtype

First off, a pet peeve of mine: while 1 makes no semantical sense. You want while True.

However, in your case you actually want while ct_changed:

ct_changed = True
while ct_changed:
    ct_changed = False
    for block in newtry:
        success, newcatchtype = block.isCatchExtendable(newhandler, catchtype)
        if not success:
            if catchtype == newcatchtype:
                break
            else:
                catchtype = newcatchtype
                ct_changed = True

Alternatively, you can flatten the nesting level by inverting the if not success conditional, and continuing:

        …
        if success:
            continue

        if catchtype == newcatchtype:
            break

        catchtype = newcatchtype
        ct_changed = True

(In fact, I’d probably go for this.)


qos method call using DateTime and Stopwatch I need to limit method call to 80 times per second (for each connectionId).

My initial version:

private const int MAX_TRANSACTIONS_PER_PERIOD = 80;
private const int PERIOD_IN_MS = 1000;
private Queue<DateTime>[] times = new Queue<DateTime>[10]
{
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100),
    new Queue<DateTime>(100)
};

private int ExecTransWithStats(int connection_id, string name, string parameters_string, byte[] bytes)
{
    DateTime nowTime = DateTime.Now;
    while (times[connection_id].Count > 0 && (nowTime - times[connection_id].Peek()).TotalMilliseconds >= PERIOD_IN_MS)
    {
        times[connection_id].Dequeue();
    }
    if (times[connection_id].Count >= MAX_TRANSACTIONS_PER_PERIOD)
    {
        Thread.Sleep(PERIOD_IN_MS - (int) (nowTime - times[connection_id].Dequeue()).TotalMilliseconds);
    }
    times[connection_id].Enqueue(DateTime.Now);

    return ExecTrans(connection_id, name, parameters_string, bytes);
}

I've decided to replace DateTime with Stopwatch just because I don't need to store entire Date.  I only need to store how many milliseconds elapsed:

private const int MAX_TRANSACTIONS_PER_PERIOD = 80;
private const int PERIOD_IN_MS = 1000;
private Queue<long>[] times = new Queue<long>[10]
{
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100),
    new Queue<long>(100)
};

private Stopwatch workingTime = Stopwatch.StartNew();

private int ExecTransWithStats(int connection_id, string name, string parameters_string, byte[] bytes)
{
    long nowTime = workingTime.ElapsedMilliseconds;
    while (times[connection_id].Count > 0 && (nowTime - times[connection_id].Peek()) >= PERIOD_IN_MS)
    {
        times[connection_id].Dequeue();
    }
    if (times[connection_id].Count >= MAX_TRANSACTIONS_PER_PERIOD)
    {
        Thread.Sleep(PERIOD_IN_MS - (int) (nowTime - times[connection_id].Dequeue()));
    }
    times[connection_id].Enqueue(workingTime.ElapsedMilliseconds);

    return ExecTrans(connection_id, name, parameters_string, bytes);
}

Am I correct that version with Stopwatch is better? It has much better precision, but I'm ok  with having precision of DateTime (10-15 ms). Do I pay for extra precision (which I don't actually need)?

StopWatch is definitely better for timing stuff, but the semantics of the two versions are different. With DateTime you have this:

private int ExecTransWithStats(int connection_id, string name, string parameters_string, byte[] bytes)
{
    DateTime nowTime = DateTime.Now;
   ...

and with StopWatch you have that:

private Stopwatch workingTime = Stopwatch.StartNew();
private int ExecTransWithStats(int connection_id, string name, string parameters_string, byte[] bytes)
{
    long nowTime = workingTime.ElapsedMilliseconds;
  ...

Bug?

The stopwatch starts as soon as the instance is created, it looks like you're misusing the stopwatch.

long nowTime = workingTime.ElapsedMilliseconds;

That's not how a stopwatch works. That will give you the number of milliseconds elapsed between the instantiation of your class and the moment ExecTransWithStats gets called. I doubt this is what you are expecting.


Toggling only a single class as active in a list with vanilla JS or CSS Every time I toggle active class in a list (to dynamically change DOM content) I find that I always go to this approach to check and clear which class is active. I'm still fairly new to using pure javascript so I'm wondering if there is a better, simpler approach?

It feels like having to run a second function to clear out all active classes seems grunty.

I've included an example of how I do it.

var specialNav = document.getElementById("special-nav");

if (specialNav) {
  var navItems = specialNav.getElementsByTagName('li');
  var i;
  for (i = 0; i < navItems.length; i++) {
    navItems[i].addEventListener("click", function() {
      if (!this.classList.contains('active')) {
        clearActives(navItems);
        this.classList.toggle('active');
      }
    });
  }
}

function clearActives(classlist) {
  if (classlist) {
    for (i = 0; i < classlist.length; i++) {
      classlist[i].classList.remove('active');
    }
  }
}
.active {
  color: red
}

li {
  cursor: pointer;
}

 1

2

3

4

Another approach could be with hidden radio inputs and :checked CSS selector :

input { display: none }
input:checked ~ * { color: red }
<input type="radio" name="li" checked>
1

<input type="radio" name="li">
2

<input type="radio" name="li">
3

<input type="radio" name="li">
4


Workflow class with Start(), Continue(), and Resume() methods I have a session class that needs to be setup based on different needs of the client code. Sometimes a new session needs to be created, or continued, or resumed (loaded from database after a user has saved their progress and came back).

Right now the client code has to ALWAYS execute Start(), Continue(), or Resume() after instantiating the WorkflowSession class or the instance is useless. Like this:

var workflowSession = new WorkflowSession(Session);
workflowSession.Continue(); // or .Continue() or .Resume()

Should I do a return this; in order to be able to chain?

var workflowSession = new WorkflowSession(Session).Continue();

Or make some type of factory or builder? If so, how?

/// <summary>
/// Manages workflow session persistence.
/// </summary>
public class WorkflowSession
{
    private int workflowPlanId;
    private int dataClipId;
    private HttpSessionStateBase session;
    private WorkflowRepository workflowRepository;
    private DataClipRepository dataClipRepository;
    private StateRepository stateRepository;

    public WorkflowSession(HttpSessionStateBase session)
    {
        this.session = session;
        workflowPlanId = (int)session[SessionKeys.WorkflowPlanId];
        dataClipId = (int)session[SessionKeys.DataClipId];

        dataClipRepository = new DataClipRepository();
        stateRepository = new StateRepository();
        workflowRepository = new WorkflowRepository();
    }

    public SchemaProvider Provider { get; set; }

    public SchemaRoot Root { get; set; }

    public DataClip Clip { get; set; }

    public WorkflowPlan Plan { get; set; }

    public WorkflowContext Context { get; set; }

    public void Continue()
    {
        Clip = dataClipRepository.GetById(dataClipId);
        Plan = workflowRepository.GetById(workflowPlanId);
        Context = ContextFactory.Current.Create(Clip, Plan);
    }

    public void Start(int workflowPlanId, int schemaId)
    {
        Provider = ProviderCache.GetProvider();
        Root = Provider.GetRoot(schemaId);
        Clip = DataClip.Create(Root, Provider);
        Plan = workflowRepository.GetById(workflowPlanId);
        Context = ContextFactory.Current.Create(Clip, Plan);

        Context.CurrentPosition = new WorkflowState(Plan.Id, Plan.Steps.First().Id, TaskStatus.Running);

        dataClipRepository.Save(Clip);
        stateRepository.Save(Context);

        this.workflowPlanId = workflowPlanId;
        this.dataClipId = Clip.Id;
        SetSessionValues();
    }

    public void Resume(int workflowPlanId, int dataClipId)
    {
        Clip = dataClipRepository.GetById(dataClipId);
        Plan = workflowRepository.GetById(workflowPlanId);
        Context = ContextFactory.Current.Create(Clip, Plan);

        this.workflowPlanId = workflowPlanId;
        this.dataClipId = dataClipId;
        SetSessionValues();
    }

    private void SetSessionValues()
    {
        session[SessionKeys.DataClipId] = dataClipId;
        session[SessionKeys.WorkflowPlanId] = workflowPlanId;
    }

    private static class SessionKeys
    {
        public const string WorkflowPlanId = "Workflow.WorkflowPlanId";
        public const string DataClipId = "Workflow.DataClipId";
    }
}

Comments:

The structure and naming of this class says "I am a runner" but it isn't really a runner if it is basically a mechanism to persist and retrieve state. This is especially confounding because you have a separate state class. There's nothing to start, continue, or resume, except storage and retrieval of state from the session. In fact, conceptually, this class is the context, and your WorkflowContext and WorkflowState classes appear to be more appropriate runner objects.
Continue() and Resume()--combine them into one method where you either always pass values, or never pass values. Either the calling class needs to be able to check the session and retrieve values to pass in, or the method needs to be changed to be able to hydrate the session if it hasn't been initialized yet. This class should probably encapsulate direct session access, so I'd go with the latter. Otherwise, calling code has to make assumptions about the session, which is brittle and an encapsulation no-no.
All void methods and a ton of properties. This class exposes a lot of objects, and more specifically, objects that this class internally initializes. This makes your void methods stateful and full of side effects. If you are using this class to basically initialize and hold a bag of properties that you then access in business logic, why not make your methods stateless by returning a state object from this method? The current class is not really suited to be both the state (session) AND the state-getter. It would be much better to move the properties on this class to a separate state object, and change this class to have CreateNewSession() and GetExistingSession() methods that then return a state object. You can probably combine that state object with your other classes, too. Again, do ensure nothing external can assume anything about the application session.


Palin Pairs (Pallindrome Counting) Code In an array of strings 'a' having 'n' strings i have to select the Palin Pairs from the given strings .for ex for input 3 bba abb abb Output=2

I am getting correct output but want to reduce time complexity....what other logic or enhancement I can use to optimize the code

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.util.Date;

public class TestClass {

public static void main(String[] args) throws Exception {
    Date d=new Date();

     BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
     ArrayList<StringBuffer> al=new ArrayList<StringBuffer>();
     int count=0;
     int no=Integer.parseInt(br.readLine());
     for (int i = 0; i < no; i++) {

     String line=br.readLine();

       al.add(new StringBuffer(line));
     }
     long ts1=System.nanoTime();
     int i=0;
     int j=0;
    for( i=0;i<al.size();i++){
        StringBuffer str=al.get(i);
        String comp=str.toString();
            for(j=i+1;j<al.size();j++){
                StringBuffer rev=al.get(j);

                String st=(rev.reverse()).toString();

                if(st.equals(comp)){
                    count++;
                }

            }
    }
    System.out.println(count);

}

}

Why don't you format your code nicely?
An IDE can easily reformat it for you,
for example, this is how IntelliJ reformats your code:

public class TestClass {

    public static void main(String[] args) throws Exception {
        Date d = new Date();

        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        ArrayList<StringBuffer> al = new ArrayList<StringBuffer>();
        int count = 0;
        int no = Integer.parseInt(br.readLine());
        for (int i = 0; i < no; i++) {

            String line = br.readLine();

            al.add(new StringBuffer(line));
        }
        long ts1 = System.nanoTime();
        int i = 0;
        int j = 0;
        for (i = 0; i < al.size(); i++) {
            StringBuffer str = al.get(i);
            String comp = str.toString();
            for (j = i + 1; j < al.size(); j++) {
                StringBuffer rev = al.get(j);

                String st = (rev.reverse()).toString();

                if (st.equals(comp)) {
                    count++;
                }

            }
        }
        System.out.println(count);

    }

}

An IDE can do far more than this,
for example warns of bad practices,
in this code:

Unused variable: d,
new ArrayList<StringBuffer>(); can be simplified as new ArrayList<>();
Unused variable ts1
Redundant initializer int i = 0 and int j = 0

After eliminating the above, import java.util.Date also becomes unnecessary, and the code is starting to become more compact.

There remain many obvious bad practices:

The loop variables i and j should be declared inside the for statements: if you do that, they cannot be used outside. Which is a good thing, as they were not intended to be used outside.
Variables should be declared as close to where they are used as possible. Similar to the previous point, the idea is to limit mistaken uses, by minimizing the "window of vulnerability". For example, int count should be declared right before the second for loop that performs the counting.
ArrayList<StringBuffer> al can be declared as List<StringBuffer> al, using the interface type List instead of a specific implementation ArrayList
Instead of StringBuffer, it's recommended to use StringBuilder. It has exactly the same interface, so you can safely replace it everywhere in your code.
Avoid throwing (or catching) Exception. Try to use the most specific exception type.
The variable names are poor throughout. The only variable that is well-named is count. All the others could have better names.

Applying the above corrections, the program becomes much better:

public static void main(String[] args) throws IOException {
    BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));
    int lineCount = Integer.parseInt(reader.readLine());

    List<StringBuilder> lines = new ArrayList<>();
    for (int i = 0; i < lineCount; i++) {
        String line = reader.readLine();
        lines.add(new StringBuilder(line));
    }

    int palindromePairCount = 0;
    for (int i = 0; i < lines.size(); i++) {
        StringBuilder str = lines.get(i);
        String comp = str.toString();
        for (int j = i + 1; j < lines.size(); j++) {
            StringBuilder rev = lines.get(j);
            String st = (rev.reverse()).toString();
            if (st.equals(comp)) {
                palindromePairCount++;
            }
        }
    }
    System.out.println(palindromePairCount);
}

We're only getting started.
There are much bigger problems with the posted code,
here are some tips to get you started:

Rework the code, replacing the List<StringBuilder> with List<String>
Extract the counting logic to a dedicated method that takes a List<String> and returns an int
Extract the palindrome checking logic to a dedicated method that takes a String and returns a boolean

These are the most obvious improvement ideas.
After you implement them,
more improvement opportunities will likely become apparent.


Attempt at method to draw diamond (recursive or not ?) I'm not so good with recursion, so I've decided to tackle an exercise consisting of drawing this pattern using characters:

      *
     ***
    *****
   *******
  *********
 ***********
*************
 ***********
  *********
   *******
    *****
     ***
      *

Here is my code so far. What could be improved? Furthermore, is recursion suitable for a task like this or not?

class Main
{
    public static void main (String[] args)
    {
        Main.drawDiamond("*", 0, 5 );
    }

    public static void drawDiamond( String seed, int turn, int centerLevel ){

        if( seed.length() <= 0 ){
            return;
        }

        char[] chars = new char[Math.abs(turn-centerLevel-1)];
        Arrays.fill(chars, ' ' );
        String spaces = new String(chars);

        System.out.print( spaces );

        System.out.println( seed );

        if( turn <= centerLevel ){
            seed += "**";
        } else {
            if( seed.length() >= 3 ) {
                 seed = seed.substring(1,seed.length()-1);
            } else {
                seed = "";
            }
        }

        turn++;

        drawDiamond( seed, turn, centerLevel );
    }
}

You can make this a recursion exercise with two parts:

1) Write a recursive method to repeat a char n times.

Signature:

public static String getCharRepeatedTimes(final char c, final int times)

(Read at end to see a solution)

2) Write a recursive method to create a diamond pattern.

Signature for recursive start method:

public static String getDiamondWithSize(final int size)

Signature for recursive "work" method:

private static String getDiamondWithSize(final int size, final int currentSize)

Hint 1: You can use the method from 1)
Hint 2: Think about a pattern in the diamon and how to exploit it.

(Read at end to see a solution)

Let us start with 1):

public static String getCharRepeatedTimes(final char c, final int times) {
    if (times == 0)
        return "";
    return String.valueOf(c) + getCharRepeatedTimes(c, times - 1);
}

And continue with 2):

public static String getDiamondWithSize(final int size) {
    return getDiamondWithSize(size, 2 - (size & 1));
}

The start method can be done in two ways. Starting from the outer area to the inner area or vice-versa. I have done the out->in way. If you allow even and odd diamonds, then we have to manage the start or end size in the correct way. There the 2 - (size & 1) part, which is 2 for even size and 1 for odd size.
The idea is now to create the outer lines first and append it to the next inner part then. Until the inner part is the full part.

private static String getDiamondWithSize(final int size, final int currentSize) {
    if (currentSize == size)
        return getCharRepeatedTimes('*', size) + "\n";
    final int nonDiamondSpace = (size - currentSize) / 2;
    final String currentLine = getCharRepeatedTimes(' ', nonDiamondSpace) + getCharRepeatedTimes('*', currentSize) + "\n";
    return currentLine + getDiamondWithSize(size, currentSize + 2) + currentLine;
}

The + concatenation could be replaced by StringBuilder , insert and append. For clarity and the exercise purpose, it is not done here.

Suggestions for your code:

public static void drawDiamond(String seed, int turn, final int centerLevel) {
    ...
    if (turn <= centerLevel)

Noone would expect this if the method is called. At least for me, it is unclear how the argument centerLevel is related to the size of the diamond. You should add at least some JavaDoc, I would prefer to change the behavior.

    turn++;
    drawDiamond(seed, turn, centerLevel);

In general, you should avoid to change arguments, if possible. In this case, we can go with this:

    drawDiamond(seed, turn + 1, centerLevel);

This looks a bit complex:

    if( turn <= centerLevel ){
        seed += "**";
    } else {
        if( seed.length() >= 3 ) {
             seed = seed.substring(1,seed.length()-1);
        } else {
            seed = "";
        }
    }

You should add at least some comments. And you can decrease the branch depth by one:

    if (turn <= centerLevel) //until the mid is reached, increase by two stars
        seed += "**";
    else if (seed.length() >= 3) //until the end is reached, reduce by 2 chars
        seed = seed.substring(1, seed.length() - 1);
    else //finished
        seed = "";

Rest is more a subjective view. I would rather introduce a method to generate a char/String repeated n times than doing it inside the diamond method. And I would avoid the seed argument as shown above.


Force ConcurrentDictionary in a Singleton Registry to Collect Removed Items/Space I have the following class which is used in a WebApi AspNetCore application as a singleton instance to store Strategy objects (reference type). The class is

/// <summary>
/// Class to allow singleton access to a common thread safe registry of registered strategies.
/// </summary>
public class StrategyRegister : IStrategyRegister
{
    private ConcurrentDictionary<long, Strategy> _registery;

    public StrategyRegister()
    {
        _registery = new ConcurrentDictionary<long, Strategy>();
    }

    public bool TryAdd(Strategy strategy)
    {
        if (_registery.ContainsKey(strategy.Id))
            throw new SystemException("The registry can never contain two identical strategy ids");
        return _registery.TryAdd(strategy.Id, strategy);
    }

    public bool TryRemove(long id)
    {
        if (!_registery.ContainsKey(id))
            return true;

        //GC.Collect();
        return _registery.TryRemove(id, out _);
    }

    public Strategy TryGet(long id)
    {
        if (_registery.ContainsKey(id))
            return _registery[id];
        return null;
    }

    public IEnumerable<Strategy> GetRegisteredStrategies()
    {
        return _registery.Select(kvp => kvp.Value);
    }
}

The problem is, as the ConcurrentDictionary grows large and items are removed, they are not being collected by the GC. I confirm this by adding the crude and nasty line GC.Collect() after every removal. When this is done, memory does not grow... I could make this "smarter" (still crude), by adding a timer to force collection every n seconds, but is there a better way, or should I be doing something obvious here?

Apparently, collections are optimized for performance over memory management. So I'm afraid it is up to us, consumers of the .NET Framework, to implement memory management on collections that grow big and require lots of manipulations.

I would use a scheduled task to periodically make a new instance of the queue, with the values of the previous instance, and clearing the old instance.


mysqli wrapper class I made the following class to wrap mysqli for PHP using prepared statements. It seems to work well, but I was hoping to get opinions (on overall structure, performance, usage, etc.). Thanks for the insight.

<?php
class database {
    private $conn, $stmt, $arr, $eof=true, $error;

    public function __construct($host, $user, $pass, $db) {
        $this->conn = mysqli_connect($host, $user, $pass, $db);
        if(mysqli_connect_errno()) $this->error = mysqli_connect_error();
    }

    public function __destruct() {
        if($this->stmt) mysqli_stmt_close($this->stmt);
        mysqli_close($this->conn);
    }

    /** Executes a prepared SQL query on the database
      *
      * The second argument (optional) is the types list, followed by the parameters
      * e.g. query('SELECT * FROM table WHERE id = ? OR name = ?', 'is', 5, 'Joe')
      *
      * @param string $query is the sql statement to execute
      * @return mixed false on error,
      *               true on successful select, or
      *               the number of affected rows
      */
    public function query($query) {
        // create a new prepared statement
        $stmt = mysqli_prepare($this->conn, $query);
        if(!$stmt) {
            $this->error = mysqli_error($this->conn);
            return false;
        }

        // apply the arguments if any exist
        if(func_num_args() > 2) {
            $args = array_slice(func_get_args(), 1);
            $refs = array();
            foreach($args as $key=>&$value) $refs[$key] = &$value;
            call_user_func_array(array($stmt, 'bind_param'), $refs);
        }

        // run the query
        $stmt->execute();
        if($stmt->errno) {
            $this->error = mysqli_error($this->conn);
            return false;
        }

        // if the query was not a select, return the number of rows affected
        if($stmt->affected_rows > -1) {
            $rows = $stmt->affected_rows;
            mysqli_stmt_close($stmt);
            return $rows;
        }

        // close any previous statement
        if($this->stmt) mysqli_stmt_close($this->stmt);
        $this->stmt = $stmt;
        $this->eof = false;

        // bind the results to the associative array
        $this->arr = array();
        $refs = array();
        $meta = mysqli_stmt_result_metadata($stmt);
        while($column = mysqli_fetch_field($meta)) {
            $refs[] = &$this->arr[str_replace(' ', '_', $column->name)];
        }
        call_user_func_array(array($stmt, 'bind_result'), $refs);

        // make the first result set available
        $this->next();
        return true;
    }

    // fetches the next row
    public function next() {
        if($this->stmt) {
            $ret = mysqli_stmt_fetch($this->stmt); // populates $this->arr
            $this->eof = ($ret !== true);
            if($ret === false) $this->error = mysqli_error($this->conn);
        }
    }

    // returns an associative array of the results
    public function results() {
        // must make a copy when returning the entire array because
        // the array holds references that may be updated
        $ret = array();
        foreach($this->arr as $key=>$value) $ret[$key] = $value;
        return $ret;
    }

    // return the value for the specified field
    public function result($field) { return $this->arr[$field]; }
    public function __get($field) { return $this->result($field); }

    // returns true if eof has occured or an error
    public function eof() { return $this->eof; }

    // returns the number of rows in the result set
    public function count() { return ($this->stmt ? $this->stmt->num_rows : 0); }

    // returns the last error message, if clear is true, clears the error as well
    public function error($clear=false) {
        $err = $this->error;
        if($clear) $this->error = '';
        return $err;
    }
}
?>

Instead of assigning your properties null or empty values from the start, just define them with no values. If they aren't initialized, then they will resolve to null. Although, from what I can tell, you aren't doing much checking of these properties anyways, so I don't understand why you had default values anyways. You really should be verifying these properties before you use them.

private
    $conn,
    $stmt,
    $arr,
    $eof=true,
    $error
;

You should always specify the access level of your methods and properties (public, private, protected). You did fairly well with your properties, but then you ignore your methods. Sure, right now PHP defaults to making each of these methods public by default, but this will be deprecated before too much longer. Plan for the future and define them properly. Besides, this is just good coding practice.

Always, always, always use braces on your statement. In languages such as Python, where braces don't exist, this is fine, but in PHP this is wrong. Even though PHP allows this syntax, even they say it is wrong, because it CAN cause issues with the code. Not to mention problems with reading the code. Its two characters,  and doesn't hurt anything to add them, but it could cause all kinds of problems by not adding them. You do this quite a bit, so I wont point out each case.

if(mysqli_connect_errno()) { $this->error = mysqli_connect_error(); }

When you have a lot to say in the comments, don't use single line comments, use block comments. Or better yet, since you seem to be documenting the behavior of a method here, you should be using PHPDoc comments.

/* returns false on error, true on successful select, number of affected rows otherwise
at minimum, query is the sql statement to execute
if there are query parameters, the second argument is is the types list, followed by the parameters
e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)*/
//OR PHPDoc Comments
/** Queries database
  *
  * The second argument is is the types list, followed by the parameters
  *
  * e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)
  *
  * @param string $query At minimum, query is the sql statement to execute if there are query parameters
  *
  * @return mixed false on error, true on successful select, number of affected rows otherwise
  */

You lied to us in the documentation and promised us a third and, at the very least, fourth parameter in your query() method. But they are no where in sight. I do see that you have decided to use func_get_args() in there, but that's a big no-no (at least for something so simple). If you want to pass an unspecified number of parameters, do so as an array. That's all you are really getting  with func_get_args() anyways. Either way, your documentation should properly reflect your code, otherwise it is useless. Besides the parameter issue, there is something fundamentally wrong with your query() method. It is entirely too busy. Two very common principles that are always associated with OOP are the "Single Responsibility" (SR) Principle and the "Don't Repeat Yourself" (DRY) Principle, the first isn't normally shortened to that acronym, but I will do so in this answer to avoid repeating myself. These two principles go hand in hand, and you have potentially violated both of them.

SR tells me that this method should only be focused with querying the database and returning the results, after all, that's what you called it query(). However, you are doing so much more. Such as binding the parameters and executing the statement. I wont explain each deviation to you, because you need to be able to separate your code properly, but those two should help get you started. I'm also not entirely certain, but is it necessary for that array to be assigned values by reference? I didn't feel like following it all the way through to make sure.

You have potentially violated the DRY principle because each of the tasks I listed above, and each of the others I didn't, could potentially be reused in this class. Instead they are hardcoded into the one method and therefore their potential is being wasted.

Here's another DRY violator. If you have two methods performing the same task, either A) get rid of one of them, or B) have one of them point to the other. This way if you ever decided to change which array you were fetching from, you would only have to do so once. This is one of the key benefits of DRY.

function result($field) { return $this->arr[$field]; }
function __get($field) { return $this->result($field); }

Ternary functions, unless nested (which they should never be), don't require parenthesis. So, this could be a stylistic point, but figured I'd let you know.

function count() { return $this->stmt ? $this->stmt->num_rows : 0; }


Making multiple calls to API and processing data in Node.JS I have written a chunk of Node that is acting as a web hook for a GitHub repository I have.  Basically when a commit is made to that repo, a request gets made to my function which then looks for potentially sensitive data that has been checked in.  I'm making multiple calls to the GitHub API to pull down data.  I tried to handle all of these calls by using the async package's waterfall and each functions.

There are two sections to the waterfall call: getting the commit data and then getting the file data.  Within the "file data" section I have the async each that is getting the actual file contents for each file.

I'm wondering if anyone can see any obvious best practices that I am not following for JavaScript programming.  Am I handling the asynchronous calls the correct way?  I created two helper functions to try and keep things clean/readable - should I break any other pieces out into their own functions?

var https = require('https')
var async = require('async')

module.exports = function (ctx,cb) {
  console.log("**********************************************")
  var danger = ['password','pass','pw','pwd','passwd','key','user','username','secret']
  var re = new RegExp(danger.join('|'))
  async.waterfall([
      function(callback){
        var options = {
          host: 'api.github.com',
          path: getApiPath(ctx.data.head_commit.url),
          headers: {
            'User-Agent': 'Broham'
          }
        }
        getData(options, function(commitData){
          // console.log("commit data", commitData)
          callback(null,commitData.files)
        })
      },
      //for each file, get the file content and look for potentially sensitive information that has been commited
      function(files, callback){
        // console.log("files", files)
        var review = []
        async.each(files, function(file, fileCallback){
          var fileOptions = {
            host: 'api.github.com',
            path: file.contents_url.replace('https://api.github.com', ''),
            headers: {
              'User-Agent': 'Broham'
            }
          }
          // get the JSON that will have the URL for the raw file content
          getData(fileOptions, function(fileData){
            // get the raw file content
            https.get(fileData.download_url, function(contentData){
                contentData.setEncoding('utf-8')
                var contents = ''
                contentData.on('data', function (data) {
                  contents += data
                })
                contentData.on('end', function(){
                  console.log('contents', contents)
                  // check to see if there are any matches for the terms we want to review.
                  matches = contents.match(re)
                  if(matches.length > 0){
                    // console.log("matches", matches)
                    review = review.concat(contents.match(re))
                  }
                  fileCallback()
                })
              })
          })
        }, function(err){
          callback(null, review)
        })

      }
    ], function(err, results){
        if(results.length > 0){
          console.log("For review:", results)
        }
        cb(null, 'GitEye complete!')
        console.log("**********************************************")
  })
}

function getData(options, callback){
  https.get(options, function(res){
    res.setEncoding('utf-8')
    var body = ''
    res.on('data', function (data) {
      body += data
    })

    res.on('end', function(){
      // console.log('body', body)
      var data = JSON.parse(body);
      callback(data)
    })
  })
}

function getApiPath(url){
  var pieces = url.split('/')
  var user = pieces[3]
  var repo = pieces[4]
  var commit = pieces[6]
  // return "https://api.github.com/repos/" + user + "/" + repo + "/commits/" + commit
  return "/repos/" + user + "/" + repo + "/commits/" + commit
}

Looking for sensitive terms

I see that you build a regex from a list of terms:

  var danger = ['password','pass','pw','pwd','passwd','key','user','username','secret']
  var re = new RegExp(danger.join('|'))

With this approach,
some of the items in danger are redundant,
because "password" and "passwd" will be already matched by "pass",
"pwd" will be already matched by "pw",
and "username" will be already matched by "user".

Overall this approach looks rather primitive and I suspect it will yield too many false positives to be useful.

Duplicated calls

In this code contents.match(re) is executed twice:

  matches = contents.match(re)
  if(matches.length > 0){
    // console.log("matches", matches)
    review = review.concat(contents.match(re))
  }

That's a waste, it would be better to use matches in place of the second call.

Truthy values in JavaScript

Non-empty arrays are truthy, so instead of if(matches.length > 0) you could write simply if (matches.length), and even just if (matches).
The same goes for if(results.length > 0) later in the code.


Sorting and eliminating duplicates From Hacker Earth:

Problem Statement:
Chotu's father is the owner of a Vada Pav shop. One Sunday, his father takes him to the shop. Father tells him that at the end of the day, Chotu has to give him a list consisting of the names of all the customers on that day who bought Vada Pav(s) from the shop. The list should not have the names of any of the customers being repeated and it should be such that the lexicographically smallest name comes first, ie., the names should be sorted in dictionary order.
As and when a particular customer buys a Vada Pav, Chotu writes down the name of that particular customer. Chotu's initial list is ready, but, he is confused as to how to make the list Father expects from him. Chotu comes to you for help. Your job now is to create the final list, as Father expects from Chotu.
Input :
First line consists of N, the number of names of customers in Chotu's initial list. The next N lines are such that each line consists of a customer's name.
Output :
On the first line, print the total number of names appearing in Chotu's final list. Then print the list such that every customer's name is printed on a new line.
Example
Sample Input:
11
babu
anand
rani
aarti
nandu
rani
rani
ap
anand
babu
nandu

Sample Output:
6
aarti
anand
ap
babu
nandu
rani

For above problem I've submitted the following code:
import java.util.*;
class TestClass {
public static void main(String args[] ) throws Exception {
  Scanner in = new Scanner(System.in);
    Set<String> set = new TreeSet<>();
    int n = in.nextInt();
    for(int i=0;i<n;i++){
        set.add(in.next());
    }
    System.out.println(set.size());
    Iterator i = set.iterator();
    while(i.hasNext()){
        System.out.println(i.next());
    }
}
}

But from 7 test cases, it passed 4 test cases but in rest of the test cases, the time limit exceeds. So my questions is how can I reduce time for this? I was thinking to use BufferedReader to read the whole input as a single String and than splitting it into a String array but the input is new-line separated not space separated. So I can't understand how to use fast I/O method here.

Why make it so complicated? Just use basic java 8 streaming operations and the new-ish BufferedReader.lines() method:

public static void main (String[] args) {
    Set<String> uniqueOrderedNames = new TreeSet<>();
    try(BufferedReader br = new BufferedReader(new InputStreamReader (System.in))){
        br.lines().forEach(uniqueOrderedNames::add);
    } catch (Exception e) {
        // don't do this, use the most specific exception type and handle it
    }
    System.out.println(uniqueOrderedNames.size());
    uniqueOrderedNames.forEach(System.out::println);
}

That much for your approach. Before you now copy paste that, I have some more things to say:

Indent correctly: Methods are indented by one level, blocks are indented 1 level. Levels add up. Your method is missing one level of indentation and your created Scanner should be indented 2 more spaces
Add spaces around binary operators: for(int i=0;i<n;i++){ becomes for (int i = 0; i < n; i++) {, which is significantly easier on the eyes.
Use clear names Set set is nonsensical, it doesn't shed any light whatsoever on what the thing does. Give it a proper name so you can understand what happens.


Validating only attributes that are present in a form I want to skip validations based on attributes. If certain attributes are not in form so I do not want to validate them.

Currently, if I post them from one form which has 1 field, it still validates other 9 fields and show me errors that it cant be blank.

If I check from big form which has 10 fields, it passes successfully.

The below code did trick for me but my main intention is to refactor so it will fullfil its purpose as i stated above.

Code is:

validates :teacher_number, presence: true, if: "teacher_number && teacher_number.blank?"
  validates :title, presence: true, if: "title && title.blank?"
  validates :name, presence: true, if: "name && name.blank?"
  validates :gender, presence: true, if: "gender && gender.blank?"
  validates :location, presence: true, if: "location && location.blank?"
  validates :dob, presence: true, if: "dob && dob.blank?"
  validates :contact_mobile, presence: true, if: "contact_mobile && contact_mobile.blank?"
  validates :contact_home, presence: true, if: "contact_home && contact_home.blank?"
  validates :street, presence: true, if: "street && street.blank?"
  validates :city, presence: true, if: "city && city.blank?"
  validates :state, presence: true, if: "state && state.blank?"
  validates :zip_code, presence: true, if: "zip_code && zip_code.blank?"
  validates :country, presence: true, if: "country && country.blank?"
  validates :teacher_number, uniqueness: {scope: :school_id}, if: "teacher_number && teacher_number.blank?"
  validate  :teacher_number_existance, :on => :create, if: "self.teacher_number && self.teacher_number.blank?"
  validate  :school_existance, :on => :create, if: "self.teacher_number && self.teacher_number.blank?"

I simply wants to do something like:

before_validation :skip_validations

def strip_validations
    [:teacher_number, :title, :name, :gender, :location, :dob, :contact_mobile, :contact_home, :street, :city, :state, :zip_code, :country].each do |attr|

      errors.add("Teacher", " #{attr} can't be blank") if attr && attr.blank?

    end
  end

But above code does not work for my cases. Any good work around?

You just need to change the method call from skip_validation to strip_validations.

before_validation :strip_validations

    def strip_validations
      ['teacher_number', 'title', 'name', 'gender', 'location', 'dob', 'contact_mobile', 'contact_home', 'street', 'city', 'state', 'zip_code', 'country'].each do |attr|

        errors.add("Teacher", " #{attr} can't be blank") if send(attr.to_sym) && send(attr.to_sym).blank?

      end
    end

for more specific error messages you can use

errors.add("Teacher", {attr => " #{attr} can't be blank"}) if send(attr.to_sym) && send(attr.to_sym).blank?


Counting votes for candidates in a category and region With the following code, I want to count the Vote objects from a collection, filtering on one of their fields, Candidate.

I have the feeling that I can get this done much more efficiently.

// Get all votes from the database
Collection<Vote> votes = getVotesFromCategoryAndRegion(c, r);

// Filter distinct candidates
Collection<Candidate> candidates = votes
            .stream()
            .map(Vote::getCandidate)
            .distinct()
            .collect(Collectors.toList());

List<VoteResult> results = new ArrayList<>();

// If vote.candidate == candidate, candidates's vote counter ++
for (Candidate currentCandidate : candidates) {
      int currentVoteCount = 0;
      for (Vote v : votes)
          if (v.getCandidate().equals(currentCandidate))
              currentVoteCount++;

      results.add(new VoteResult(currentCandidate, currentVoteCount));
    }

    Collections.sort(results);
    return results;

If you have any suggestions, please tell.

can get this done much more efficiently.

Yes, I agree.

If C candidates each receive an average of V votes (total of C * V votes cast), then your complexity is quadratic in number of candidates: O(C * C * V).

There's nothing so bad about making a pair of linear passes, to accumulate candidates and then votes.

But there's no need, you can make a single pass to do both. Create a HashMap, iterate over all votes, probe the map to see if you should add a zero entry for a new candidate, and then increment the entry so you'll have vote totals at the end.


Custom tableview cell for displaying different foods I have a custom tableview cell for displaying different foods, with a star next to each name. If the food has been favorited (i.e., it exists in the database), the star is filled in, otherwise it is empty.  I'm not sure if the logic that determines the color of the star should go in the view controller or the tableviewcell.

Currently, I have this in the view controller under didSelectRowForIndexPath:

cell.foodLabel.text= food.name;
if ([cell isStarredForName:food.name])
        [cell changeStarToState:StateFull];
else
        [cell changeStarToState:StateEmpty];

Also, I use this cell in two different view controllers, so I have to use the code above twice.

In the cell, I have:

- (BOOL) isStarredForName:(NSString*)foodName {

    PFInstallation *installation = [PFInstallation currentInstallation];
    NSArray *foodList = (NSArray*)[installation objectForKey:@"favorites"];
    return ([foodList containsObject:foodName]);
}

- (void)changeStarToState:(StarState)state{
    NSString *imgName;
    if (state == StateEmpty){
        imgName = @"star_none.png";
        _currentState = StateEmpty;
    }
    else {
        imgName = @"star_full.png";
        _currentState = StateFull;
    }
    [self.favButton setImage:[UIImage imageNamed:imgName] forState:UIControlStateNormal];

}

I'm wondering if it's better to just have a [cell handleStarColor] function in the viewcontroller and have the cell decide whether it needs to fill in the star.

I can't talk much about the big picture issue that you are asking about, but I see plenty of improvements that can be made to the code.

When working with Objective-C, it is considered best practice to follow Apple's guidelines and conventions for the language.  One of these is to use the full name of the enum when naming its contents.  I see you have the enum StarState and then you have StateEmpty and StateFull.  These should be changed to StarStateEmpty and StarStateFull.  Sometimes this can lead to really long names, but it is still Apple's recommendation to do things this way.  The biggest reason why is that if two names conflict, the compiler will not be able to determine which one is which.  This is a real problem here, because if you ever want to have fill states for another type of object, you will have problems trying to use just StateFull.  Another option would be to change the name of the enum to ObjectFillState and then you can reuse it for other objects.

If I remember correctly, in a previous question you were advised to fix your irregular white spacing, and to also always use brackets around if-else statements.  It is better to always have brackets so that the code is more readable.

if ([cell isStarredForName:food.name])
    [cell changeStarToState:StateFull];
else
    [cell changeStarToState:StateEmpty];

There are lots of ways that a construct like the above could go wrong in the future, so it is better to just put in the brackets.  Also this cell.foodLabel.text= food.name; is missing a space after text.  A minor nitpick but things like that are still important for readability.

I have a feeling that there is a much better way to go about retrieving the information from the dictionary here (such as an object that returns the proper value from a properly named method):

PFInstallation *installation = [PFInstallation currentInstallation];
NSArray *foodList = (NSArray*)[installation objectForKey:@"favorites"];
return ([foodList containsObject:foodName]);

But I will specifically mention this part: (NSArray*)[installation objectForKey:@"favorites"].  Someone here on Code Review once said this to me, and I think it is very good advice: "The most common purpose of a cast is to turn a compile-time error into a run-time error."  Please click the link and read his expanded explanation of this quote.  I think it is very good advice.

I do not know completely if it is best practice in this case or not, but I am not a fan of uninitialized values such as this: NSString *imgName;.  There is a bigger problem with the method - (void)changeStarToState:(StarState)state however.  The name says that it changes the state of the star, but then it also includes this line:

[self.favButton setImage:[UIImage imageNamed:imgName] forState:UIControlStateNormal];

At the very least the method name should be changed to reflect that the image is being set also.  However, I'm thinking that this is actually an indication of a bigger design problem.  It makes me think that you may want to change your approach. Since that is the reason you posted this question in the first place, I think you are on the right track.  Hopefully someone else will be able to elaborate on the design part of your question.