How to optimize jQuery to run on asp.net web form updatepanel? I wrote this Jqeury to make it work with asp.net update panel. I know i do little bit of hacking to make it work just. I want to get some help in how to optimize and remove the hacks from my script.

<script type="text/javascript">

    //get all elemnt with class .resource-download
    $.each($(".resource-download"), function () {
        var fileExtension = getFileExtension(this.href);
        if (fileExtension == "" || fileExtension != "pdf") {
            $(this).addClass("hideLinke");
        }
    });

    //load script agine
    function init() {
        $.each($(".resource-download"), function () {
            var fileExtension = getFileExtension(this.href);
            if (fileExtension == "" || fileExtension != "pdf") {
                $(this).addClass("hideLinke");
            }
        });
    }

    // getFileExtension
    function getFileExtension(fileURL) {
        if (fileURL.indexOf('.') === -1) { return ""; } //check for no extension
        return fileURL.split('.').pop();
    }

    //loadGridLayout
    function loadGridLayout() {
        $('#grid-wrapper').masonry();
        $('.selectpicker').selectpicker();

        $('.btn-hamburger').click(function () {
            $('.lts-nav').toggleClass('expanded');
            $('.wrapper').toggleClass('wrapper-expanded');
        });

        /* iPhone Site Search Styling */
        var searching = false;

        $('.search-add-on').click(function () {
            if ($(window).width() < 480) {
                $('.section-1 .search-hideable').toggle();
                $('.section-1 .site-search').focus();
                searching = true;
            }
        });

        $('.site-search').blur(function () {
            if ($(window).width() < 480) {
                $('.section-1 .search-hideable').toggle();
                searching = false;
            }
        });

        $(window).resize(function () {
            if ((searching || !$('.site-search').is(':visible')) && $(window).width() > 480) {
                $('.section-1 .search-hideable').toggle(true);
                $('.section-1 .search-hideable').removeAttr('style');
                searching = false;
            }
        });
    }
    //add script to script manager
    var prm = Sys.WebForms.PageRequestManager.getInstance();
    prm.add_endRequest(init);
    prm.add_endRequest(loadGridLayout);

</script>

Some notes in no particular order:

1

//get all elemnt with class .resource-download
$.each($(".resource-download"), function () {
    var fileExtension = getFileExtension(this.href);
    if (fileExtension == "" || fileExtension != "pdf") {
        $(this).addClass("hideLinke");
    }
});

//load script agine
function init() {
    $.each($(".resource-download"), function () {
        var fileExtension = getFileExtension(this.href);
        if (fileExtension == "" || fileExtension != "pdf") {
            $(this).addClass("hideLinke");
        }
    });
}

better to define the function and call it rather than copy paste the code.

function init() {
    $.each($(".resource-download"), function () {
        var fileExtension = getFileExtension(this.href);
        // see point 2
        if (fileExtension == "" || fileExtension != "pdf") {
            $(this).addClass("hideLinke");
        }
    });
}

init();

2

if (fileExtension == "" || fileExtension != "pdf") {

Is pointless, "" !== "pdf" so you should just have the second condition.

if (fileExtension !== "pdf") {

3
It's better to be in the habit of using === and !== to avoid subtle bugs caused by type coercion.

4

$.each($(".resource-download"), function ()  {}); is better written as $(".resource-download").each(function() {});


Country data archiving script using Google Spreadsheet I've just created a script in Google Spreadsheet in order to archive some things. I have data from four countries.  If something is marked done, then that line could be archived into that country's archive.

My code is pretty ugly. Is there a clearer way to do this?

function CroatianArchive()
{
  var source_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('original');
  var target_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('croatian_backup');
  var lastRow = source_sheet.getLastRow();
  var source_range = source_sheet.getDataRange();
  var target_range = target_sheet.getDataRange();
  var rowsToBeDeleted = [];

  var i = 2;
  while (i <= lastRow) {
  if (source_sheet.getRange("A"+i).getValue() == "CRO" && source_sheet.getRange("M"+i).getValue() == "DONE" ) {
    var office = source_sheet.getRange("A"+i).getValue();
    var title = source_sheet.getRange("B"+i).getValue();
    var imdbId = source_sheet.getRange("C"+i).getValue();
    var channel = source_sheet.getRange("D"+i).getValue();
    var type = source_sheet.getRange("E"+i).getValue();
    var added = source_sheet.getRange("F"+i).getValue();
    var deadline = source_sheet.getRange("G"+i).getValue();
    var airing = source_sheet.getRange("H"+i).getValue();
    var link = source_sheet.getRange("I"+i).getValue();
    var picture = source_sheet.getRange("J"+i).getValue();
    var comment = source_sheet.getRange("K"+i).getValue();
    var portId = source_sheet.getRange("L"+i).getValue();
    var status = source_sheet.getRange("M"+i).getValue();
    var data = [office,title,imdbId,channel,type,added,deadline,airing,link,picture,comment,portId,status];
    target_sheet.appendRow(data);
    rowsToBeDeleted.push(i);
    i++;
    } else {
      i++;
      }
    }
    rowsToBeDeleted.reverse();
    for (var j = 0; j < rowsToBeDeleted.length; j++)
    {
        source_sheet.deleteRow(rowsToBeDeleted[j]);
    }
    SpreadsheetApp.getActiveSpreadsheet().toast('Archivation finished.', 'Status');
  }
function SerbianArchive()
{
  var source_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('original');
  var target_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('serbian_backup');
  var lastRow = source_sheet.getLastRow();
  var source_range = source_sheet.getDataRange();
  var target_range = target_sheet.getDataRange();
  var rowsToBeDeleted = [];

  var i = 2;
  while (i <= lastRow) {
  if (source_sheet.getRange("A"+i).getValue() == "SER" && source_sheet.getRange("M"+i).getValue() == "DONE" ) {
    var office = source_sheet.getRange("A"+i).getValue();
    var title = source_sheet.getRange("B"+i).getValue();
    var imdbId = source_sheet.getRange("C"+i).getValue();
    var channel = source_sheet.getRange("D"+i).getValue();
    var type = source_sheet.getRange("E"+i).getValue();
    var added = source_sheet.getRange("F"+i).getValue();
    var deadline = source_sheet.getRange("G"+i).getValue();
    var airing = source_sheet.getRange("H"+i).getValue();
    var link = source_sheet.getRange("I"+i).getValue();
    var picture = source_sheet.getRange("J"+i).getValue();
    var comment = source_sheet.getRange("K"+i).getValue();
    var portId = source_sheet.getRange("L"+i).getValue();
    var status = source_sheet.getRange("M"+i).getValue();
    var data = [office,title,imdbId,channel,type,added,deadline,airing,link,picture,comment,portId,status];
    target_sheet.appendRow(data);
    rowsToBeDeleted.push(i);
    i++;
    } else {
      i++;
      }
    }
    rowsToBeDeleted.reverse();
    for (var j = 0; j < rowsToBeDeleted.length; j++)
    {
        source_sheet.deleteRow(rowsToBeDeleted[j]);
    }
    SpreadsheetApp.getActiveSpreadsheet().toast('Archivation finished.', 'Status');
  }

You have a lot of repeated code when creating the data array. Note that the names of each cell value are ultimately irrelevant. That part could also be expressed as:

var columns = ["A" // office
              ,"B" // title
              ,"C" // imdbId
              ,"D" // channel
              ,"E" // type
              ,"F" // added
              ,"G" // deadline
              ,"H" // airing
              ,"I" // link
              ,"J" // picture
              ,"K" // comment
              ,"L" // portId
              ,"M" // status
              ];
var data = columns.map(function (col) {
  return source_sheet.getRange(col + i).getValue();
});

Less redundant code, same level of documentation.

Code like

if (...) {
  ...;
  i++;
} else {
  i++;
}

is usually better written as:

if (...) {
  ...;
}
i++;

However, we now have

var i = 2;
while (i <= lastRow) {
  ...;
  i++;
}

which is a more complicated formulation of

for (var i = 2; i <= lastRow; i++) {
  ...;
}

Similarly,

rowsToBeDeleted.reverse();
for (var j = 0; j < rowsToBeDeleted.length; j++)
{
    source_sheet.deleteRow(rowsToBeDeleted[j]);
}

is the same as:

for (var i = rowsToBeDeleted.length - 1; i >= 0; i--) {
  source_sheet.deleteRow(rowsToBeDeleted[i]);
}

If I see that correctly, your two function only differ in the strings croatian_backup vs. serbian_backup and CRO vs. SER. Instead of copy-pasting that code, take those strings from the function parameters. Now we have a general archive function:

function archive(outputSheet, languageCode) { ... }

If you need two seperate funtions that don't take any arguments, we can use the currying technique (also known as partial application) to pre-fill the arguments:

function CroatianArchive() {
  return archive("croatian_backup", "CRO");
}

Some of your names use underscores, other capitalization to separate words:

source_range
lastRow

You should settle for one style – Javascript tends to prefer capitalization rather than underscores.


Sending user information to server to track hits / traffic on site This is working pretty well right now:

var p='';
if(typeof parameters!=='undefined'){p='&p='+parameters;}
var t='';
var l='ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
for(var i=0;i<64;i++){t+=l.charAt(Math.floor(Math.random()*62));}
if(!sessionStorage.t){sessionStorage.t=t;}
if(!localStorage.t){localStorage.t=t;}
var r=new XMLHttpRequest();
r.open('POST','https://example.com/hit',true);
r.setRequestHeader('Content-type','application/x-www-form-urlencoded');
r.send('r='+document.referrer+'&u='+localStorage.t+'&s='+sessionStorage.t+p);

I have two questions.

About 1% of my hits are getting r, the referrer, correctly, while u and s are NULL. u is like a user id to keep track of unique users, and s is a session id to track the user's session. Are these NULL because of lack of web storage support in the user's browser, or is there another explanation?
Are there any other ways to improve this? Do you see any potential problems?

p is for custom parameters that I need for my individual sites. I already ignore requests from user agents that look like bots. I am pretty sure that the 1% is real users.

For one, I wouldn't work with that code. I would prefer working on a verbosely written code. If you want to shrink this piece of code, use a minifier before release. But on development, work on the full, verbosely-written code.

That said, variable names could be better. p, u, t, s tell me nothing about what these variables are. Best if you expand the names to ones that mean something.

Also, your variables are in the open, in the global scope. There is a slight chance your code would be clobbered by other code on the page. Wrap it in a closure to avoid collision.

Now for your approach.

XMLHttpRequest which is a bit overkill. It's not cross-browser friendly if you factor in older browsers, and you're using a POST request. If you just want to track hits, a "tracking pixel" is much simpler.

It's the same approach, but instead of XMLHttpRequest, you use an image. You create the object, tack the data as query params, append that image to the page to kick the request. Your server receives the data as a GET request, gathers the data, and have it redirect to a dummy 1x1 image as a response, literally a "pixel".

For your ID logic, you will not have 100% ID'ing with your current approach. Clearing the cache will force your logic to regenerate an ID, a totally different ID from previous sessions. This will disconnect the user from any known previous data, making your data erroneous.

Consider generating a hash from pieces of browser data that almost never change. Examples of which are the browser's name, the OS name, the timezone, the screen resolution, browser settings, and so forth. A good place to start is the browser's navigator object.

The name of the technique is "device fingerprinting". The fun thing about this approach is you never have to store the value. Since the values are always present but never change, generating a hash from the same values will always yield the same value.

;(function(){

  function hashFunction(string){
    // Any will do. See http://stackoverflow.com/a/7616484/575527
  }

  var userId = hashFunction([
    navigator.userAgent,
    navigator.oscpu,
    // Add more...
  ].join(''));

  var image = new Image();
  var path = 'https://example.com/hit';
  var params = [
    'r=' + encodeURIComponent(document.referrer),
    'u=' + encodeURIComponent(userId),
  ].join('&');

  document.body.appendChild(image);
  image.src = encodeURI(path + '?' + params);

}());

Taking this approach further, you won't need JavaScript at all. HTTP requests send in non-changing data via headers. There will also be some form of non-changing data via the network, like the request IP address. You can move all of this logic to the back-end, using that data to construct a hash. The page request is sufficient to get the data even.


Ruby coin toss probability tool I haven't been programming for a while, I wanted to do a simple project to warm myself up and get back into the game; I made this simple tool that finds the probabilities of coin toss combinations. Enter a number to get all combinations with that many flips, or enter a string to search for that combination, i.e. H**H would find all combinations starting and ending with heads.

module Every
  def every(&block)
    arys = map do |ary|
      base_ary = eval ary.inspect.split(':')[1] rescue nil
      base_ary.nil? ? ary : base_ary
    end
    longest = arys.max_by(&:count).count

    arys.map!.with_index do |ary, i|
      cyc = ary * ( longest / ary.count + 1 )
      self[i].is_a?(Enumerator) ? cyc[0...longest] : ary
    end
    arys.transpose.each &block
  end
end

class Array
  # I didn't have to use this module, but I had written it in the past
  # and this seemed like a good place to use it
  include Every
end

class CoinToss
  attr_accessor :outcomes, :reference, :iterations, :combins, :occrs

  def initialize(outcomes = ["H", "T"], reference = gets.chomp.upcase)
    @outcomes = outcomes.each &:to_s
    if (Integer reference rescue false)
      @iterations = reference.to_i
      @reference = "*" * @iterations
    else
      set = @outcomes.join("*")
      raise ArgumentError, "Invalid Characters" unless reference =~ /\A[#{set}]+\z/
      @reference = reference
      @iterations = @reference.length
    end

    @combins = @outcomes.repeated_permutation(@iterations).to_a
    @occrs = 0

    ref = [@reference.chars]
    # every loop ref is repeated but combins is iterated
    [ref.cycle, @combins].every do |r, c|
      # replaces `*` in ref with regex string for characters from `outcomes`
      @template = r.join.gsub "*", "[#{@outcomes.join}]"
      @occrs += 1 unless !(c.join =~ /#{@template}/) # +1 if it matches the template
    end
  end

  def get_combins(match_marker = "~")
    @combins.each do |c|
      # m is blank if combin doesn't match the template
      m = !(c.join =~ /#{@template}/) ? "" : match_marker
      line = Array c.join
      # skips marker when reference contains only `*`
      line << m unless @reference.chars.all? { |r| r == "*" }
      puts line.join(" ")
    end
  end

  def get_probability(rationalize = true, match_word = "out of")
    puts Rational(occrs, combins.length) if rationalize
    puts [@occrs, match_word, @combins.length].join(" ") unless rationalize
  end
end

tosses = CoinToss.new
tosses.get_combins
tosses.get_probability(rationalize = false)
tosses.get_probability(rationalize = true)

Thanks for any advice.

Here are some suggestions to your code
Every module
I had quite a hard time to understand what it does.
Using eval can be quite dangerous so I would try to avoid it.
eval ary.inspect.split(':')[1] rescue nil

You used a module to monkey patch the Array class which is sensible. Another, even more safe way to monkey patch could be to use refinements.

Refinements are designed to reduce the impact of monkey patching on other users of the monkey-patched class. Refinements provide a way to extend a class locally.

https://docs.ruby-lang.org/en/2.4.0/syntax/refinements_rdoc.html
The safest way of course is to not use monkey patching at all. Maybe something like array.flatten.each could do the trick too?
CoinToss
Initialize reference
Initializing the reference in your constructor looks quite complicated. Maybe it could get extracted to dedicated classes.
class Reference
  def initialize(input, allowed_characters = [])
    @input = input
    @allowed_characters = allowed_characters
  end

  def to_s
    if integer?
      IntegerReference.new(input.to_i)
    else
      StringReference.new(input, allowed_characters)
    end
  end

  private

  attr_accessor :input, :allowed_characters

  def integer?
    Integer(reference)
  rescue
    false
  end
end

class IntegerReference
  def initialize(size)
    @size = size
  end

  def to_s
    build_string
  end

  def valid?
    true
  end

  private

  attr_accessor :size

  def build_string
    "*" * size
  end
end

class StringReference
  def initialize(string, allowed_characters = [])
    @string = string
    @allowed_characters = allowed_characters
  end

  def to_s
    return string if valid?
  end

  def valid?
    string =~ /\A[#{validation_regex}]+\z/
  end

  private

  attr_accessor :string

  def validation_regex
    allowed_characters.join("*")
  end
end

Use getter
You already setup attribute accessors but then fail to use them. Instead of @combins use combins. This has the advantage that you can change the way combins is returned by changing the getter method.
Return data instead of puts
Return data from your methods instead of using a puts. This has several advantages likes

Use the classes outside a terminal (e.g. website)
Easier to test

Naming
There are only two hard things in computer science ... Try to use speaking names and avoid abbreviations like combins, occrs, r, c. This will help to make your program easier to read and understand.
Lazy loading
Instead of setting up everything in the controller, use lazy loading and memoization.
def combinations
  @combinations ||= outcomes.repeated_permutation(@iterations).to_a
end

def iterations
  @iterations ||= reference.length
end


Find the no of pairs whose sum divides the product from 0 - 1000000000 I have designed the code but the problem is it timeouts as the range is 1000000000 but works fine with 1000. Any idea to optimize this or a mathematic logic for it? I just need to find the no of pairs(a,b) such that a < b and a*b % a+b == 0 if a range is given,
eg: 15 - o/p 4 as (3,6)(4,12)(6,12)(10,15).

php code:

$count = 0;
$no = 1000;
for($b = 1; $b <= $no; $b ++) {
    for($a = 1; $a < $b; $a ++) {
        $sum = $a + $b;
        $prod = ($a * $b);

        if (($prod % $sum == 0) {
            ++ $count;
        }
        $arr [$b] = $count;
    }
}

echo '<pre>';
print_r($arr);
echo '<pre>';

I need some math logic to get no of pairs rather performing actual process as above. If I found a relation or expression for this then actual process is not required.

These kind of puzzles have to be solved by first analyzing the problem.
If you look at the samples that you have already found you will notice that in all samples the two numbers share a rather large common factor. This makes sense, since if a and b have a common factor then both a+b and a*b are divisible by this common factor, which increases the probability that a+b divides a*b. So let's try to find out more formally when (a,b) is a good pair:

Let g=gcd(a,b). Then there exist integers r,s with

a = r * g
b = s * g

Then

a + b = (r+s)*g
a * b = r*s*g^2

Thus a+b divides a*b if (r+s)*g divides r*s*g^2 and hence if r+s divides r*s*g.
Also since g is the greatest common divisor it follows that r and s have no divisor in common (i.e. gcd(r,s)=1). From Euclid's algorithm follows that gcd(r+s,r) = 1 and
gcd(r+s, s) = 1 and hence also gcd(r+s, r*s) = 1. Thus if r+s divides  r*s*g then
r+s must divide g.

Thus all the good pairs below a bound N can be described as follows:
If r,s,k are integers such that 1 < r < s < N^0.5, gcd(r,s)=1 and s * (r + s) * k <= N then (a,b) = (r * (r + s) * k, s * (r + s) * k) is a good pair.

Implementing this isn't hard. Running the algorithm up to 10^9 takes a few minutes (depending on the programming language).

One big question remains: Why on earth has the problem been migrated to "Code review"?


Simple Caesar shifter in C# I use these in a windows form with fun buttons, I wanted to see what the world thinks of my coding.

This is something that I coded about 2-3 years ago, I did change some of the coding when I looked at it and thought "this could be better written like this"

Let me know what you think and what I could do differently.

Public class shiftCipher
{
    public string removeSpaces(string plainIn)
    {
        plainIn = plainIn.Replace(" ", string.Empty);
        return plainIn;
    }

    public string reverse(string plainIn)
    {
        char[] charArray = plainIn.ToCharArray();
        Array.Reverse(charArray);

        return new string(charArray);
    }

    public string toUCase(string strIn)
    {
        strIn = strIn.ToUpper();
        return strIn;
    }

    public string shift(string plainIn, int shift)
    {// 65=a 90=z in ASCII
        string readOut=string.Empty;
        char[] charArray;

        charArray = plainIn.ToCharArray();
        for (int i = 0; i < plainIn.Length; i++)
        {
            int Num = Convert.ToInt32(charArray[i]) + shift;
            readOut += Convert.ToChar(Num > 90 ? Num -= 26 : (Num < 65 ? Num += 26: Num));
        }
        return readOut;
    }

    //this method will take all of these methods and put them together to encrypt
    public string Encrypt(string ReadIn, int Shift)
    {
        ReadIn = removeSpaces(ReadIn);
        ReadIn = reverse(ReadIn);
        ReadIn = toUCase(ReadIn);
        ReadIn = shift(ReadIn, Shift);
        return ReadIn;
    }

    public string Decrypt(string ReadIn, int amountShift)
    {
        ReadIn = reverse(ReadIn);// undo the reverse by running the method again
        ReadIn = shift(ReadIn, -amountShift);

        return ReadIn;
    }
}

In the Decrypt I didn't think there was a good way to put the spaces back into the string, so I didn't try to take that on, and I also left the string in uppercase if it was there.

Simpler Methods.

the code:

public string removeSpaces(string plainIn)
{
    plainIn = plainIn.Replace(" ", string.Empty);
    return plainIn;
}

Could be:

public string removeSpaces(string plainIn)
{
    return plainIn.Replace(" ", string.Empty);
}

Which begs the question... why have the removeSpaces method at all?

The same is true with toUCase()

Simpler Conversion

Why use Convert.ToInt32(....); and Convert.ToChar(...) when (int) and (char) will do?

Simpler Math

This line here (the crux of the problem), can be simplified using modulo and removing the Magic Numbers:

int Num = Convert.ToInt32(charArray[i]) + shift;
readOut += Convert.ToChar(Num > 90 ? Num -= 26 : (Num < 65 ? Num += 26: Num));

can be:

// Constants....
private const int LETTERA = (int)'A';
private const int LETTERCOUNT = (int)'Z' - LETTERA + 1;

// actual code.
 readOut += (char)(LETTERA + (((int)charArray[i] + shift - LETTERA + LETTERCOUNT) % LETTERCOUNT));

(See ideone here)

Without the conditional / ternary operator it is more likely to be tighter/faster code as well.

Comments

You have used magic numbers, so you attempted to document them with a comment:

public string shift(string plainIn, int shift)
{// 65=a 90=z in ASCII

But, the comments are wrong, and misleading (wrong case character).

 {// 65=A 90=Z in ASCII

Symmetry

When you have methods that do symmetrical work (they are closely related), then you should ensure the parameter names are the same.... For example, you have:

public string Encrypt(string ReadIn, int Shift)

and

public string Decrypt(string ReadIn, int amountShift)

The second should have the parameter name Shift


Code with many "early returns (exits)" into the functional style I have some Scala code that uses Internet to authorize a user. Therefore, it can throw Exceptions like IOException in the method.

The original code was written in Java. So, yes, it uses variables, not val. Also it "returns early" many times if some condition is not satisfied.

The following is an example. (The actual code is, of course, a little more complicated.)

def connect(): (IdInfo, PasswordInfo, ConnectionInfo) = {
  var idInfo: IdInfo = null
  var pwdInfo: PasswordInfo = null
  var connInfo: ConnectionInfo = null

  try {
    idInfo = calcIdInfo()
  } catch {
    case ex: IOException => return null
  }

  try {
    pwdInfo = calcPwdInfo(idInfo)
  } catch {
    case ex: AuthorizationException => return null
  }

  try {
    connInfo = calcConnInfo(idInfo, pwdInfo)
  } catch {
    case ex: IOException => return null
  }

  try {
    verify(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => return null
    case ex: AuthorizationException => return null
  }

  (idInfo, pwdInfo, connInfo)
}

I'm trying to rewrite the code in the functional style. What I did was

Use vals instead of vars
Remove "return early"
Use None instead of null

I tried to use methods like fold, but at each stage, the types of input or the types of output are different - IdInfo, (IdInfo, PasswordInfo), (IdInfo, PasswordInfo, ConnectionInfo) are all different types - and, what each stage does is also diffrent from each other, so I couldn't use such method.

def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
  val idInfoOpt = try {
    Some(calcIdInfo())
  } catch {
    case ex: IOException => None
  }

  idInfoOpt map (idInfo => {
    val pwdInfoOpt = try {
      Some(calcPwdInfo(idInfo))
    } catch {
      case ex: AuthorizationException => None
    }

    pwdInfoOpt map (pwdInfo => {
      val connInfoOpt = try {
        Some(calcConnInfo(idInfo, pwdInfo))
      } catch {
        case ex: IOException => None
      }

      connInfoOpt map (connInfo => {
        try {
          verify(idInfo, pwdInfo, connInfo)
          Some(idInfo, pwdInfo, connInfo)
        } catch {
          case ex: IOException => None
          case ex: AuthorizationException => None
        }
      }) getOrElse None
    }) getOrElse None
  }) getOrElse None
}

The resultant code is more complex than the original one. There are deeper indentations, and the type of each value is not easy to determine (A lot of Options and map and getOrElse).
In the above example, the code has calc***Info() methods, but in reality the code is more complex since it has more statements between each stage, so there seems almost no reason to use the actual resultant code instead of the original one.

I think that the procedural style is better than the functional style in this case. Or not? The more complex the code is, the more chance of errors there will be. Are there better ways to get rid of vars and early returns?

For ease, here are compilable source codes (although it does nothing):
The first one: http://ideone.com/KJS1tn
The second one: http://ideone.com/gulzgs

After reading the code, I assume these are the signatures of the building blocks of your code

class IdInfo
class PasswordInfo
class ConnectionInfo

def calcIdInfo:IdInfo=???
def calcPwdInfo(idInfo:IdInfo):PasswordInfo=???
def calcConnInfo(idInfo:IdInfo,pwdInfo:PasswordInfo):PasswordInfo=???

I left out the implementations for the calc as they don't really matter.

There are several possible variations to improve on the current code. Let's have a look at the last call to  map in connect  :

 connInfoOpt map (connInfo => {
    try {
      verify(idInfo, pwdInfo, connInfo)
      Some(idInfo, pwdInfo, connInfo)
    } catch {
      case ex: IOException => None
      case ex: AuthorizationException => None
    }
  }) getOrElse None

The try expression has type Option[(IdInfo, PasswordInfo, ConnectionInfo)] which is the type you want as the output of the function connect. As the name connInfoOpt suggests, it is also an option. Map on option has a signature roughly equivalent to :

map(f:A=>B):Option[B]

Where A is the type which is wrapped in the original option, and B is the type wrapped in the resulting option.

Thus let's say connInfoOpt has type Option[ConnectionInfo] we get

A = ConnectionInfo
B = Option[(IdInfo, PasswordInfo, ConnectionInfo)]

Therefore the return type of the map expression is Option[B] which can be expanded to

Option[Option[(IdInfo, PasswordInfo, ConnectionInfo)]]

Here lies the first problem : to get the Option[(IdInfo, PasswordInfo, ConnectionInfo)] out, you use getOrElse None. A slightly shorter way to express that is to call flatten.

The original expression can thus be rewritten as:

 connInfoOpt map (connInfo => {
    try {
      verify(idInfo, pwdInfo, connInfo)
      Some(idInfo, pwdInfo, connInfo)
    } catch {
      case ex: IOException => None
      case ex: AuthorizationException => None
    }
  }) flatten

which has type Option[(IdInfo, PasswordInfo, ConnectionInfo)]. The next step is to study the option type and see that it has a flatMap method which has the following signature :

flatMap(f:A=>Option[B]):Option[B]
// for comparison with map which was
map(f:A=>B):Option[B]

Then notice that each intermediary is step is of the form  :

option map { A=> Option[B] } flatten

for instance:

connInfoOpt map (connInfo => {
  try {
    verify(idInfo, pwdInfo, connInfo)
    Some(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => None
    case ex: AuthorizationException => None
  }
}) flatten

can be rewritten as :

connInfoOpt flatMap (connInfo => {
  try {
    verify(idInfo, pwdInfo, connInfo)
    Some(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => None
    case ex: AuthorizationException => None
  }
})

Which means the whole function can now be simplified to :

 def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
    val idInfoOpt = try {
      Some(calcIdInfo())
    } catch {
      case ex: IOException => None
    }

    idInfoOpt flatMap (idInfo => {
      val pwdInfoOpt = try {
        Some(calcPwdInfo(idInfo))
      } catch {
        case ex: AuthorizationException => None
      }

      pwdInfoOpt flatMap (pwdInfo => {
        val connInfoOpt = try {
          Some(calcConnInfo(idInfo, pwdInfo))
        } catch {
          case ex: IOException => None
        }

        connInfoOpt flatMap (connInfo => {
          try {
            verify(idInfo, pwdInfo, connInfo)
            Some(idInfo, pwdInfo, connInfo)
          } catch {
            case ex: IOException => None
            case ex: AuthorizationException => None
          }
        })
      })
    })
  }

We got rid of the 3 getOrElse but this still isn't so nice. Let's extract a few methods next to make the code easier to read :

def safeIdInfo():Option[IdInfo]={
  try {
    Some(calcIdInfo())
  } catch {
    case ex: IOException => None
  }
}

def verifyConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo, connInfo: ConnectionInfo) :Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
  try {
    verify(idInfo, pwdInfo, connInfo)
    Some(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => None
    case ex: AuthorizationException => None
  }
}

def safeConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo): Option[ConnectionInfo] = {
  try {
    Some(calcConnInfo(idInfo, pwdInfo))
  } catch {
    case ex: IOException => None
  }
}

def safePwdInfo(idInfo: IdInfo): Option[PasswordInfo] = {
  try {
    Some(calcPwdInfo(idInfo))
  } catch {
    case ex: AuthorizationException => None
  }
}

then the connect method looks like :

def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
  safeIdInfo() flatMap (idInfo =>
    safePwdInfo(idInfo) flatMap (pwdInfo=>
      safeConnInfo(idInfo, pwdInfo) flatMap (connInfo =>
        verifyConnInfo(idInfo, pwdInfo, connInfo)
      )
    )
  )
}

Which is better but not perfect. Let's apply some scala syntactic sugar :

def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
  for{
    idInfo   <- safeIdInfo()
    pwdInfo  <- safePwdInfo(idInfo)
    connInfo <- safeConnInfo(idInfo, pwdInfo)
    verified <- verifyConnInfo(idInfo, pwdInfo, connInfo)
  } yield verified
}

The connect method is much better (I think) but we end up with repetition in the "safe" methods. Actually, Scala offers a type to reduce this duplication, the Try type which we will use to replace the Option type in the "safe" methods :

def verifyConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo, connInfo: ConnectionInfo) :Try[(IdInfo, PasswordInfo, ConnectionInfo)] = Try {
  verify(idInfo, pwdInfo, connInfo)
  (idInfo, pwdInfo, connInfo)
}
def safeIdInfo():Try[IdInfo]=Try(calcIdInfo())
def safeConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo): Try[ConnectionInfo] = Try(calcConnInfo(idInfo, pwdInfo))
def safePwdInfo(idInfo: IdInfo): Try[PasswordInfo] = Try(calcPwdInfo(idInfo))

Of course doing this, the connect method is no longer valid at the type level it needs to be adapted. To match the exact behavior of the original code the adaptation will look like :

  def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
    val tryVerified = for{
      idInfo <- safeIdInfo()
      pwdInfo <-  safePwdInfo(idInfo)
      connInfo <- safeConnInfo(idInfo, pwdInfo)
      verified <- verifyConnInfo(idInfo, pwdInfo, connInfo)
    } yield verified

    tryVerified match {
      case Success(value) => Some(value)
      case Failure(authException:AuthorizationException)=> None
      case Failure(ioException:IOException)=> None
      case Failure(failed)=> throw failed
    }
  }

However I will suppose that you don't really want to rethrow unknown runtime exceptions and instead want to return None for any error, in which case the following form of connect should work :

  def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
    val tryVerified = for{
      idInfo <- safeIdInfo()
      pwdInfo <-  safePwdInfo(idInfo)
      connInfo <- safeConnInfo(idInfo, pwdInfo)
      verified <- verifyConnInfo(idInfo, pwdInfo, connInfo)
    } yield verified

    tryVerified.toOption
  }

The complete code is now :

 def verifyConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo, connInfo: ConnectionInfo) :Try[(IdInfo, PasswordInfo, ConnectionInfo)] = Try {
    verify(idInfo, pwdInfo, connInfo)
    (idInfo, pwdInfo, connInfo)
  }
  def safeIdInfo():Try[IdInfo]=Try(calcIdInfo())
  def safeConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo): Try[ConnectionInfo] = Try(calcConnInfo(idInfo, pwdInfo))
  def safePwdInfo(idInfo: IdInfo): Try[PasswordInfo] = Try(calcPwdInfo(idInfo))

  def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
    val tryVerified = for{
      idInfo <- safeIdInfo()
      pwdInfo <-  safePwdInfo(idInfo)
      connInfo <- safeConnInfo(idInfo, pwdInfo)
      verified <- verifyConnInfo(idInfo, pwdInfo, connInfo)
    } yield verified

    tryVerified.toOption
  }

Using the right types, a functional style and some syntactic sugar can go a long way to simplify your code :)

For further reference, check the scala doc for Option and Try, also check out the FAQ on yield for more explanations on the for-expression syntactic sugar.


PHP Trait constructors I've been rewriting parts of my own framework and eventually came back to where it all began, making traits more useful and implementing auto initializing traits and I am wondering if someone has a bright idea on how to implement it a bit cleaner.

The idea of this is simple, let traits have constructors that are called before the class __construct().

namespace myframework\traits{
  trait Construct{
    function __wakeup(){
      foreach(array_remove(__TRAIT__, ref(class_uses(self::class, false))) as $trait){
        $list[($v = \myframework::loader()->trait($trait))[0]][] = $v[1];
        # generates: $list[$priority][] = $callback
      }

      if(ksort($list)){
        foreach($list as $cb){
          foreach($cb as $fn){
            $fn->call($this);
          }
        }
      } else {
        # Something horrible happend, oh well can/does it ever fail?
      }
    }
  }

  trait DB{
    private $db;
  }

  # Register anon function to fire when trait gets loaded. Constructor.
  \myframework::loader()->trait('myframework\traits\DB', function(){
    $this->db = myframework\Database::getInstance();
  }, 0);

  # This function essentially makes:
  # $data['myframework\traits\DB'] = [0 => 0, 1 => callback]
  # where key index 0 is the load priority, which gets sorted.
}

I tried to trim the code as much as possible for simplicity, the array_remove() is a function returning the an updated array by removing a key from a generated array and ref() is just a wrapper function to allow array_remove() to be inlined.

Now lets assume the above code is autoloaded, making a new file and doing the following is now possible:

class someClass{
  use myframework\traits\Construct;
  use myframework\traits\DB;

  public function __construct(){
    // DRY concept, $this->db is already set.

    $stmt = $this->db->prepare("SELECT * FROM table");
  }
}

Now this is just a minor example, imagine if you add a trait Log adding a method log($msg) that will automatically log the message under the class name and with the auto initializer the object itself can determine where to log, for example the filesystem if the database failed to connect.

So my question mainly is, if there is a cleaner way to implement the anonymous function because it is currently registered by name instead of reference. Also since this is the core of the framework it needs to be efficient as possible because __wakeup() can be called for each object created using this system.

Also I'd like to have some input on what I could improve upon it.

I've been working on it and I feel I finally have it like I wanted to.

Instead of using the __wakeup() function, I'm binding a closure to the new object that dispatches other closures whom are registered to a traits name. So as a result I don't even need the Constructor trait anymore.

Not only that, hooking this all up to an event system I was able to create destructors as well with an added bonus that I can use __wakeup() and __sleep() as they where designed for.

I'm reusing code by using the event system, so in summary its now more dynamic, shorter code and somewhat faster in execution


Processing.js physics simulation I've been fooling around in JavaScript again, and I've come up with this simple physics simulation. I think it has a pretty simple setup, here's "psuedocode" of sorts to explain it.

Constructor PhysicsEntity

Private method _calculateForces - Calculates forces to be applied.
Private method _applyForces - Apply the calculated forces.
Private method _renderEntity - Render the PhysicsEntity at it's position.
Method updateEntity - Update the entity.

I think the interface is pretty clear, but if you have any suggestions, they would be greatly appreciated.

/*
A simple physics object for managing
certain things such as forces acting
upon the object, and rendering.
*/
var PhysicsEntity = function(mass, entityWidth, entityHeight, accelerationVector, position, lifeTime)
{
    this.mass = mass;
    this.entityWidth = entityWidth;
    this.entityHeight = entityHeight;
    this.accelerationVector = accelerationVector;
    this.position = position;
    this.velocity = new PVector(0, 0);

    // Calculate the forces to be applied to the velocity
    this._calculateForces = function()
    {
        this.velocity.add(this.accelerationVector.x*this.mass, this.accelerationVector.y*this.mass);
    };

    // Apply velocity to the Entity's position
    this._applyForces = function()
    {
        this.position.add(this.velocity);
    };

    // Render the Entity at it's position
    this._renderEntity = function()
    {
        rect(this.position.x, this.position.y, this.entityWidth, this.entityHeight);
    };

    // Update the entity
    this.updateEntity = function()
    {
        this._calculateForces();
        this._applyForces();
        this._renderEntity();
    };
};

Here's an example of how this can be used.

// Data for creating new PhysicsEntities and storing them
var DEFAULT_MASS = 1;
var DEFAULT_WIDTH = 10;
var DEFAULT_HEIGHT = 10;
var DEFAULT_ACCELERATION = new PVector(0, 0.2);
var DEFAULT_LIFETIME = 250;
var entityArray = [];

// Create a new PhysicsEntity at the mouse
var createEntity = function()
{
    entityArray.push(
        new PhysicsEntity(
            DEFAULT_MASS, DEFAULT_WIDTH, DEFAULT_HEIGHT,
            DEFAULT_ACCELERATION, new PVector(mouseX, mouseY),
            DEFAULT_LIFETIME
        )
    );
};

// Update all entities
var updateEntities = function()
{
    for(var n = entityArray.length-1; n >= 0; n--)
    {
        var entity = entityArray[n];
        entity.updateEntity();
    }
};

// Main draw loop
draw = function() {
    background(255, 0, 0);
    createEntity();
    updateEntities();
};

function Constructor(){
  this._isMyPrivates = function(){}
}

The usual problem with this "function inside this constructor to emulate private" approach is that every instance of the constructor will have copies of the functions. This will eat up your memory fast, especially that your object sounds like a base class for physics objects.

Try using prototypal inheritance instead.The methods are defined once, and shared rather than defined for each instance.

// Constructor
var PhysicsEntity = function(mass, entityWidth, entityHeight, accelerationVector, position, lifeTime){
    this.mass = mass;
    this.entityWidth = entityWidth;
    this.entityHeight = entityHeight;
    this.accelerationVector = accelerationVector;
    this.position = position;
    this.velocity = new PVector(0, 0);
};

// Prototype
PhysicsEntity.prototype._calculateForces = function(){}
PhysicsEntity.prototype._applyForces = function(){}
PhysicsEntity.prototype._renderEntity = function(){}
PhysicsEntity.prototype.updateEntity = function(){}

// This means that
// instance (where props live) -> prototype parent (where methods live)
//                                      A
// another instance (where props live) -' // literally pointing to the same object

And don't worry about "private". One can easily tamper the JS anyways, one way or another. You can prefix privates with underscores, as it's the usual convention.


A class to add and select players within a database I have a question and want to know if my class is considered object oriented.

class Player {

    private $db

    function __construct($db) {

        $this->db = $db;

    }

    public function set_player($name, $position, $number, $team) {

        try {

            $sql = 'INSERT INTO players SET playername = :playername, position = :position, teamid = :teamid, jersey = :jersey';
            $this->db->prepare($sql);
            $s->bindValue(':playername', $name);
            $s->bindValue(':teamid', $team);
            $s->bindValue(':position', $position);
            $s->bindValue(':jersey', $number);
            $s->execute();

        } catch (PDOEXception $e) {

            echo 'Could not add player, please try again.';
            exit();

        }

    }

    public function get_player($id) {

        $result = $this->db->query('SELECT * FROM players WHERE id =' . $id);
        $row = $result->fetch(PDO::FETCH_OBJ);
        return $row;

    }

}

In another file  I will use this class in this way

include_once('./includes/global.php');

$player = new Player($pdo);

$player->set_player(Chris, Forward, 13, 1);

echo $player->get_player(1)->playername;
echo $player->get_player(1)->position;

Calling get_player twice is not a good idea, as each time a db query is necessary.

As you are asking about OOP: You could add an actual Player object, which contains playername, position, etc as fields. Your current class could then be renamed PlayerDAO or something, and would then return a player object.

The main advantage would be that a concrete object is easier to use than an anonymous one (classes are to some degree self-documenting, while you can't be sure what an anonymous object actually contains).

Misc

don't die or echo in classes, it makes code less flexible and harder to reuse, as the calling code cannot try to recover.
get_player and set_player could be renamed to get and set as the class is already called player.
It would probably also be good to rename set to save or similar, as it makes more explicit that there is some kind of database interaction (set sounds as if a private field is set).


A simple Java game where the player guesses the word It's a simple word guessing game. The game picks a random word - you guess a letter until it's correct, the game automatically starts a new one.

Is my code readable? Am I doing anything that could be simplified?
Am I doing the right thing by using only one class besides Main?

import java.util.Scanner;
import java.util.Random;

class Word {
  private String censoredWord = "";
  private String word = "";
  private String[] words = {"cow", "giraffe", "deer", "cat", "dog",   "cheese"}; ////words

  public String randomizeWord() { //randomizes word
      Random rand = new Random();
      int randNum = rand.nextInt(words.length);
      word = words[randNum];
      for (int i = 0; i < word.length(); i++) { //makes a word filled with blank spots the same length as the word chosen
        censoredWord = censoredWord + "_";
      }
      return word;
  }

  public String showProgress(char letter) { //uncensors letters
    char[] wordArray = word.toCharArray(); //sets the word to an array so it can loop though it
    char[] censoredWordArray = censoredWord.toCharArray();
    for (int i = 0; i < wordArray.length; i++) {
      char ichar = wordArray[i];
      if (letter == ichar) {
        censoredWordArray[i] = ichar; //fills in censored letter
      }
    }
    censoredWord = String.valueOf(censoredWordArray);
    System.out.println(censoredWord);
    return censoredWord;
  }
}

class Main {
  public static void main(String[] args) {
    Word word = new Word();
    String currentWord = word.randomizeWord();
    while (true) { //handles the game structure and guessing
      System.out.println("Guess!");
      Scanner guessLetter = new Scanner(System.in); //takes input
      char letterGuessed = guessLetter.nextLine().charAt(0); //puts input into variable
      //checks if user got the word
      guessLetter.close();
      if (word.showProgress(letterGuessed).equals(currentWord)) {
        System.out.println("Hooray! Resetting...");
        word = new Word();
        currentWord = word.randomizeWord();
      }
    }
  }
}
```

Performance

You create a new Scanner for every guess. this is unnecessary. the same scanner can be used to read all user input. the scanner should be closed only at the end of the program. and if you adopt that, you can also use Java 7 try-with-resources feature. it is a safe(r) way to open and process external resources.

JDK library features

in order to fill an array with the same character, you can use Arrays.fill() method: Arrays.fill(censoredWordArray, '_'); and you do not need to keep the String representation of the array, just print the string value to the user. However, FYI, Java 11 gives us String::repeat to fill a String with one character: String censoredWord = "_".repeat(word.length());
I assume this is CS course homework, so I do not know if this comment is applicable. However, Java 8 (which has been around for over 5 years) gave us stream processing of collections, which, compared to for loops, is a more expressive (and sometimes performant) way to iterate over collections and arrays. for example, the loop in showProgress can be re-written like this

IntStream.range(0, wordArray.length)
    .filter(i -> wordArray[i] == letter)
    .findFirst()
    .ifPresent(i -> censoredWordArray[i] = letter);

Best practices

Avoid magic numbers and literals:
private static final String EMPTY_LETTER = "_";
not only it makes the code more readable, it will eliminate typo bugs in case you need to specify the literal in multiple places in the code.


Arduino Sketch for sending sensor turn on/off to computer I'm having this Arduino Sketch:

#define maxDigitalPin 7 //musn't be greater than 127

boolean state[] = { false, false, false, false, false, false };
unsigned long nextTime[] = { 0, 0, 0, 0, 0, 0 };

void setup(){
  Serial.begin(9600);

  for(int i = 2; i <= maxDigitalPin; i++){
    pinMode(i, INPUT_PULLUP);
  }
}

void loop(){
  for(int i = 2; i <= maxDigitalPin; i++){
    int arrayIndex = i - 2;
    if(digitalRead(i) == LOW && !state[arrayIndex] && millis() > nextTime[arrayIndex]){
      Serial.write(i ^ 128);
      state[arrayIndex] = true;
    }
    else if(state[arrayIndex] && digitalRead(i) == HIGH){
      Serial.write(i);
      state[arrayIndex] = false;
      nextTime[arrayIndex] = millis() + 500;
    }
  }
}

It will send a byte to a computer when a sensor goes on and when it goes back off.

The byte looks like:

SPPP PPPP

S is 1 when the sensor has gone on and 0 when it has gone off. P is the pin.

This code prevents sending the on message for a pin for 500ms after it has gone off. This is intended and really necessary.

My concerns:

Is it possible to reduce the code to only use one array?
I fear this sketch will be slow once it's using more than 7 pins because it's really important that the computer gets informed pretty instantly.

Of course if there is anything else to improve I would like to hear it too.

If I understand correctly, your goal is to report all state changes; a 500 ms blackout is undesirable. If I am correct, read on.

You don't need the nextTime array at all. Change the state array from false/true to LOW/HIGH:

level = digitalRead(i);
if (level != state[arrayIndex]) {
    state[arrayIndex] = level;
    serialWrite(i | (level << 8));
}

(I am assuming that LOW == 0, and HIGH == 1).

Regarding performance: the code runs a busy loop, which is usually undesirable, especially in the embedded world. I seriously recommend to connect your pins to an interrupt, and rewrite the code into a reactive model.


Returning a collection of unique elements: List using Contains() or HashSet I'm writing a binary PowerShell module and have recently started to add wildcard support to some cmdlets.

I'm not sure if I'm using an efficient way of creating the final collection of objects to write back to the pipeline.

I'm wondering if it might be slightly more efficient to use a HashSet<T> rather than a List<T> in this case, even though I've been told that Contains() doesn't have a large overhead on small collections (the final collection would can contain anywhere from 0 to ~25 elements).

Is List<T> appropriate in this case or should I try and use a HashSet<T> instead?

[Cmdlet(VerbsCommon.Get, "DatabaseUser")]
[Alias("gdbu")]
[OutputType(new Type[] { typeof(User)})]
public class GetDatabaseUserCmdlet : PSCmdlet
{
    [Parameter(ValueFromPipeline = true,
               ValueFromPipelineByPropertyName = true,
               ValueFromRemainingArguments = true)]
    public string[] Name { get; set; }

    [Parameter]
    public SwitchParameter IncludeSystemAccounts{ get; set; }

    List<User> _users = new List<User>();
    Hashtable _privateData;
    Server _server;
    Database _database;

    protected override void BeginProcessing()
    {
        _privateData = (Hashtable)MyInvocation.MyCommand.Module.PrivateData;
        _server = new Server((string)_privateData["Server"]);
        _database = _server.Databases[(string)_privateData["Database"]];
    }

    protected override void ProcessRecord()
    {
        var users = _database.Users.Cast<User>();

        if(IncludeSystemAccounts.IsPresent == false)
        {
            users = users.Where(user => user.IsSystemObject == false);
        }

        if (Name == null)
        {
            _users = users.ToList();
        }
        else
        {
            foreach(var item in Name)
            {
                var wildcard = new WildcardPattern(item, WildcardOptions.IgnoreCase);
                var result = users.Where(user => wildcard.IsMatch(user.Name));
                if(result != null)
                {
                    foreach(var user in result)
                    {
                        if(_users.Contains(user) == false)
                        {
                            _users.Add(user);
                        }
                    }
                }
            }
        }
    }

    protected override void EndProcessing()
    {
        WriteObject(_users);
    }
}

To give an example of how this cmdlet functions. Assume my database has 10 users: named User1 through User10 (not counting dbo, sys, etc.).

If I call the cmdlet like so:

Get-DatabaseUser -Name user[135], user[2-4] | Format-Table -Property Name

This is the expected (and actual result):

Name
----
User1
User3
User5
User2
User4

Checking if an item exists in a list or a hashset has very different order of complexity:

\$O(N)\$ in a list: all items are checked one by one until a match is found. If no match, the entire list will be scanned
(amortized) \$O(1)\$ in a hashset: calculate hashcode, check if it exists

With today's computers, the performance with ~25 elements should be virtually the same. Even if you had a 1000 elements, you might still not notice a difference. If in doubt, measure it.

Note that converting the list to a hashset would have the consequence of losing the ordering. An alternative could be a SortedSet, if you don't mind sorting instead of ordering. For a data structure that preserves ordering and the time complexity of hashset operations, you could consider a linked hashset implementation discussed for example here.

To conclude, if you know you will only ever have few entries (< 1000),
the performance difference won't be noticeable.
However, as a matter of principle, it might be a good idea to use a hashset anyway, especially if you don't care about the order of entries.
A sorted set would be even better, to have consistent ordering, and fewer surprises.

You use the boolean literal false at many places, for example:

if(IncludeSystemAccounts.IsPresent == false)
{
    users = users.Where(user => user.IsSystemObject == false);

It's more natural to not use boolean literals like this, but write instead:

if (!IncludeSystemAccounts.IsPresent)
{
    users = users.Where(user => !user.IsSystemObject);


A simple weather app in Node.js while practicing Promises, I ended up with a simple weather application which works on the command line. I would highly appreciate it if you can point out any flaws in my code. They can be related to the efficiency, etc. I'm looking for anything you can provide in the review.

package.json:

{
  "name": "weather-app",
  "version": "1.0.0",
  "description": "A simple weather app.",
  "main": "app.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "Hassan Althaf",
  "license": "MIT",
  "dependencies": {
    "request": "^2.67.0",
    "yargs": "^3.30.0"
  }
}

app.js:

var weather = require('./weather.js');
var location = require('./location.js');
var argv = require('yargs')
    .options({
        location: {
            demand: false,
            alias: 'l',
            type: 'string',
            description: 'Enables you to specify a location.'
        }
    })
    .help('help')
    .argv;

if(argv.location) {
    weather(argv.location).then( function(message) {
        console.log(message);
    }).catch( function(error) {
        console.log(error);
    });
} else {
    location().then( function (location) {
        return weather(location);
    }).then( function(output) {
        console.log(output);
    }).catch ( function(error) {
        console.log(error);
    });
}

location.js:

var url = 'http://ipinfo.io';
var request = require('request');

module.exports = function() {
    return new Promise( function(resolve, reject) {
        request({
            url: url,
            json: true
        }, function (error, response, body) {
            if (error || !body) {
                reject('Failed to fetch the location.');
            }

            resolve(body.city);
        });
    });
};

weather.js:

var request = require('request');

module.exports = function (location) {
    return new Promise( function(resolve, reject) {
        location = encodeURIComponent(location);

        if(!location) {
            reject('No location provided!');
        }

        var url = 'http://api.openweathermap.org/data/2.5/weather?q=' + location + '&units=metric&appid=2de143494c0b295cca9337e1e96b00e0';

        request({
            url: url,
            json: true
        }, function (error, response, body) {
            if(error) {
                reject('Failed to fetch the weather!');
            }

            resolve('It\'s ' + body.main.temp + ' degrees Celsius in ' + body.name + '!');
        });
    });
};

Let's start with weather.js.

Use Node's querystring module to generate the query string from an object instead of concatenating strings. It's easier to read, plus you don't have to deal with escaping.
is appid your API key? You shouldn't put any keys in the code. I suggest you move it out of your code and into an environment variable. That way, it's not part of the history of your version control.
url seems constant (except the query part). You should also move that outside into a variable like you did with location.js. Just combine the url with your constructed query string during calls.
I would suggest you don't construct the text for resolve in the weather module. Have it resolve the data instead. Let the caller decide what it does with the data. It's better that way in the long run, so that you can just chain tools that speak data rather than presentation.

In location.js

Reject with the error object. Let the caller decide what it wants to do with the error. Same as mentioned earlier, it's easier when the caller decides what to do with the data and for functions to just speak in data.

In app.js

You can merge both logic branches together. If you have the location, you can create a pre-resolved promise using the static function Promise.resolve(value). Otherwise, call location(). Then you can have the rest of your logic chain afterwards.

(argv.location ? Promise.resolve(argv.location) : location()).then(function(location){
  return weather(location);
}).then(function(output){
  console.log(output);
}).catch (function(error){
  console.log(error);
});


Key on a Header and Offset I have a section of code that I've been attempting to clean up and hopefully speed up. The section of code where I pass values in order to generate a key and update a table on a different sheet I think is my biggest problem. However I would like any suggestions/modifications to clean this section up first.

An overview is my worksheet has a header section where the user will update the monthly  sales,units or avg. cost field. Once this is done I need to recalculate all values in that month. This is anywhere from 10 to 1200 rows. As the update executes the following occurs:

Cell is updated with the impact of the header change.
A new unique key is built.
The new key is found in a table and the change value is inserted.

I was stuck manually changing the "ColumnCount" variable to maintain the offset in the proper position. Any help would be great.

'Table update afet net units header update
Dim i As Integer
'LastRow used to determine final used row on worksheet
    LastSourceRow = LastRow(destsheet, "B")
'Column Count initially set to -2 to offset target to desired cell
    ColumnCount = -2
    RowCount = 1

'Runs from row 1 to the last row on the sheet
For RowCount = 1 To LastSourceRow Step 5
    i = 5
    'After header change offset 5 rows and 2 columns back to capture cell change

    Set target = target.Offset(i, ColumnCount)

        'Running Status
        Application.StatusBar = "Target value change : " & target.Row & " of " & LastSourceRow & " " & Format(target.Row / LastSourceRow, "Percent")

            'If change cell is 0 then skip and sel column count to 0 to keep offset aligned
            If target = 0 Then

            'Do Nothing if 0 value
            ColumnCount = 0
            Else

                'Generate unique key and pass to table update module
                modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

                'offset target by 1 to capture change
                ColumnCount = 1

                'Set target offset to new cell
                Set target = target.Offset(, ColumnCount)

                'Generate key and pass target offset value to table update module
                modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

                'Set target offset back to original cell
                ColumnCount = -1

                Set target = target.Offset(, ColumnCount)
                'Set column count to 0 to maintain alignment when loop repeats

                ColumnCount = 0
            End If
Next RowCount

I'm doing a little guess work here because you've not shown us the method'd signature. I apologize if I've made some bad assumptions.

Typically, when I see a Target Range, it means we're working inside of a worksheet's OnChange event. Assuming that, I would recommend introducing a new Range variable into the mix so you're not accidentally modifying the incoming range. Adding an extra variable would make it clear and explicit when/if you're programmatically changing the same range the user modified.

This snippet makes me believe it's the right thing to do even if my assumption is wrong.

                'Set target offset to new cell
                Set target = target.Offset(, ColumnCount)

                'Generate key and pass target offset value to table update module
                modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

                'Set target offset back to original cell
                ColumnCount = -1

                Set target = target.Offset(, ColumnCount)
                'Set column count to 0 to maintain alignment when loop repeats

By introducing an offsetTarget variable, you should be able to avoid tracking the column count and all this setting and resetting because you've never modified target to begin with.

The other thing I would be careful of is detailed at the end of this answer and again assumes this code resides in the Worksheet_Change event. Target could possibly be a multi-cell Range. Double check your code to make sure it can deal with that edge case.

One last thing. This method takes in an awful lot of parameters.

modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

Consider creating a simple class to encapsulate that data and simplify the call.

modTableUpdate.TableUpdate instanceOfYourAwesomeNewClass


Displaying RSS feeds from Google Feed API as HTML list What it does is:

Reads from a RSS feed using Google Feed API
Shows the list in an unordered list

How good/bad is the code snippet?

$(document).ready(function(){
var FeedManager = {
        config : {
            feedContainer : $('#feedContainer'),
            feedUrl : 'http://rss.bdnews24.com/rss/english/home/rss.xml',
            feedLimit : 10
        },
        init: function(){
            var feed = new google.feeds.Feed(FeedManager.config.feedUrl);
            feed.setNumEntries(FeedManager.config.feedLimit) ;
            feed.load(function(result) {
                if (!result.error) {
                    FeedManager.$feedContainer = FeedManager.config.feedContainer;
                    for (var i = 0; i < result.feed.entries.length; i++) {
                        var entry = result.feed.entries[i];
                        $('<li/>').append(
                            $('<a>'+entry.title+'</a>').attr(
                                {
                                    'title': entry.title,
                                    'href': entry.link
                                }
                            ).bind('click',FeedManager.showStory)
                        ).appendTo(FeedManager.$feedContainer);
                    }
                }
                else{
                        FeedManager.handleError(result.error.message);
                }
            });
        },
        showStory: function(){
              var href = event.currentTarget.href;
              FeedManager.showURL(href);
              event.preventDefault();
        },
        showURL: function(url){
             if (url.indexOf("http:") != 0 && url.indexOf("https:") != 0) {
                return;
            }
            chrome.tabs.create({
                url: url
            });
        },
        handleError: function(errorText){
            $('<li/>')
                .css("color","red")
                .append("Error:"+errorText)
                .appendTo(FeedManager.config.feedContainer);
        }
    };

    FeedManager.init();
});

At the 2nd stage, I wanted to add custom accordion feature and news snippet:

processFeedResult: function(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }
    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('<li/>').append(
            $('<a>', {  // this should not be an anchor tag,right? TBD
                title: 'Published at: '+this.publishedDate,
                text: this.title
            })
        ).append($('<div>', {
            text: this.contentSnippet,
            css : {'display':'none',
                'padding-top':'2px'}
            }).append(
                $('<a>', {
                    text: '[more..]',
                    href: this.link,
                    click: FeedManager.showStory
                }))
        )
        .bind('click',FeedManager.showSnippet)
        .appendTo(FeedManager.$feedContainer);
    });
},
showSnippet: function() {
    var $obj = $(event.currentTarget),
    $snippetDiv = $obj.find('div').slideDown('normal');

    if(FeedManager.$lastOpenedDiv === undefined){
        FeedManager.$lastOpenedDiv = $snippetDiv ;
    }
    else{
        FeedManager.$lastOpenedDiv.slideUp('normal');
        FeedManager.$lastOpenedDiv = $snippetDiv ;
    }
    }
};

I feel that I have put some tightly coupled code in my processFeedResult such as text and CSS.  I also wanted to know if my showSnippet function good enough or not.  However, it works, and I know that there are 3rd party good accordion available, but I wanted to learn it.

So far I've kept the anchor tag to show the news title and I used anchor title as a tooltip that shows the time.  Maybe there is a good alternate, like span or paragraph?

Looks ok, to be honest. A few minor changes I would make:

I would pull this code out into a function

function ProcessFeedResult(result) {
    if (!result.error) {
        FeedManager.$feedContainer = FeedManager.config.feedContainer;
        for (var i = 0; i < result.feed.entries.length; i++) {
            var entry = result.feed.entries[i];
            $('
').append(
                $(''+entry.title+'').attr(
                {
                    'title': entry.title,
                    'href': entry.link
                }
                ).bind('click',FeedManager.showStory)
            ).appendTo(FeedManager.$feedContainer);
        }
    }
    else{
        FeedManager.handleError(result.error.message);
    }
}

Then I would reverse the condition and return, to reduce the level of indent a bit

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    for (var i = 0; i < result.feed.entries.length; i++) {
        var entry = result.feed.entries[i];
        $('
').append(
            $(''+entry.title+'').attr(
            {
                'title': entry.title,
                'href': entry.link
            }
            ).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    }
}

Further, I would replace the loop with jQuery's $.each()

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('
').append(
            $('' + this.title + '').attr(
            {
                'title': this.title,
                'href': this.link
            }
            ).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    });
}

You can do any or all of these, I don't think any of it is vastly important. I can read your intent easily enough, it just looks a bit neater to me after refactoring.

Thinking about your later comment a bit, I would also be tempted to append the title, rather than stringing it together.

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('
').append(
            $('').append(this.title).attr(
            {
                'title': this.title,
                'href': this.link
            }
            ).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    });
}

Otherwise this is the correct approach to generating elements, as far as I know.

As a final step, I would tidy up the braces around the attributes, for consistency

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('
').append(
            $('').append(this.title).attr({
                'title': this.title,
                'href': this.link
            }).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    });
}

I really don't think you're going to get it any cleaner than that.


Map unique raw words to a list of code words Problem

Write a function that replaces the words in raw with the words in code_words such that the first occurrence of each word in raw is assigned the first unassigned word in code_words. If the code_words list is too short, raise an error. code_words may contain duplicates, in which case the function should ignore/skip them.

Examples:

encoder(["a"], ["1", "2", "3", "4"]) → ["1"]
encoder(["a", "b"], ["1", "2", "3", "4"]) → ["1", "2"]
encoder(["a", "b", "a"], ["1", "1", "2", "3", "4"]) → ["1", "2", "1"]

Solution

def encoder(raw, code_words):
    cw = iter(code_words)
    code_by_raw = {} # map of raw item to code item
    result = []
    seen = set() # for ignoring duplicate code_words
    for r in raw:
        if r not in code_by_raw:
            for code in cw: # cw is iter(code_words), "persistent pointer"
                if code not in seen:
                    seen.add(code)
                    break
            else: # nobreak; ran out of code_words
                raise ValueError("not enough code_words")
            code_by_raw[r] = code
        result.append(code_by_raw[r])
    return result

Questions

My main concern is the use of cw as a "persistent pointer". Specifically, might people be confused when they see for code in cw?

What should be the typical best practices in this case?

Might it be better if I used the following instead?

try:
    code = next(cw)
    while code in seen:
        code = next(cw)
except StopIteration:
    raise ValueError("not enough code_words")
else:
    seen.add(code)

My main concern is the use of cw as a "persistent pointer". Specifically, might people be confused when they see for code in cw?

No. Instead, you can just remove the line cw = iter(code_words) as long as it's a native iterable. "Persistent Pointer" isn't a thing in python, because all python knows are Names.

  What should be the typical best practices in this case?

That would be building a dictionary and using it for the actual translation. You're basically already doing this with your code_by_raw, if a bit more verbose than others might. The only real difference would be that, in my opinion, it would be better to first establish the translation, and then create the result.

Except for your premature result generation, I would say your current function isn't bad. It does what it needs to do, it does it well without stupid actions, but it's not very readable. It's said often, I think you need to factor out a bit of code. Specifically, the bit that handles the fact that your inputs don't have to yield unique values, and how you need to handle duplicates.

I would suggest a  generator to handle that. This simplifies the main function a ton. (A comment pointed me towards the unique_everseen recipe, which is a slightly broader function. We don't quite need all it's functionality, but it might be worth the effort if you need some more flexibility.)

def unique(iterable):
    """ Generator that "uniquefies" an iterator. Subsequent values equal to values already yielded will be ignored. """
    past = set()
    for entry in iterable:
        if entry in past:
            continue
        past.add(entry)
        yield entry

def encoder(raw_words, code_words):
    # Create mapping dictionary:
    code_by_raw = dict(zip(unique(raw_words), unique(code_words))
    # Check if we had sufficient code_words:
    if len(code_by_raw) < len(raw_words):
        raise ValueError("not enough code_words")
    # Do translation and return the result
    return [code_by_raw[raw] for raw in raw_words]

I can't completely tell your experience level with python. For result creation, I'm using comprehensions here.

  Might it be better if I used the following instead?

It would not be bad functionally to use a structure like that, but it's still ugly (but opinions may differ). It basically does the same as my unique() generator up there.


Unit testing a commands class import itertools
import os
import re
from typing import List

class Command:
    """Base command class which all commands inherit from.

    Command class implements all common functions and utilities
    between commands, like file and directories handling, and specific
    commands like arbitrary, text, or picture commands inherit it.
    """

    def __init__(self, args):
        """Instantiate an object with the files/directory to work on."""
        self.dir_path = args.dir_path
        self._extract_files(args)
        self._files_paths = None

    def _extract_files(self, args):
        """
        Returns the files specified by the user, if any.
        Throws exception if no file matches user input.
        """
        self.files_list = None
        if args.name is not None:
            self.files_list = self._file_from_name(args.name)
        elif args.names_file is not None:
            self.files_list = self._names_from_file(args.names_file)
        elif args.rgx is not None:
            self.files_list = self._files_from_rgx(args.rgx)
        elif args.rgx_file is not None:
            self.files_list = self._rgx_from_file(args.rgx_file)
        else:
            self.files_list = self._extract_all_files()

        if len(self.files_list) == 0:
            raise FileNotFoundError("No files match")

    def _files_from_rgx(self, rgx):
        """
        Looks for files in the directory `self.dir_path` with names
        matched to the provided regex and returns those files names.
        """
        f = []
        for (dir_path, dirnames, filenames) in os.walk(self.dir_path):
            f.extend(filenames)
            break
        prog = re.compile(rgx)
        m = []
        for file_name in f:
            matched = prog.match(file_name)
            # matched.span() => (match start index, match end index)
            # Check that the file name is matched as a whole
            if matched and matched.span()[1] == len(file_name):
                m.append(file_name)
        return m

import collections
import os
import pytest

from fipy.parser import parser
from fipy.commands.command import Command

class TestCommand:
    """Tests for the command class."""

    def _populate_tmp_dir(self, tmpdir, dirs, files):
        dirs = [tmpdir.mkdir(x) for x in dirs]
        files = [tmpdir.join(x).write('') for x in files]

    def test_files_from_rgx_no_match(self, tmpdir):
        dirs = ['abc', '1abc', 'x1.txt']
        files = ['x.txt', 'x.tsf', '1abc.png']
        self._populate_tmp_dir(tmpdir, dirs, files)
        args = parser.parse_args(
            ['-p', str(tmpdir), '-r', 's[0-9]*.txt',
            'any', 'rename', '-asc']
        )
        with pytest.raises(FileNotFoundError):
            command = Command(args)

First of all, is it a good practice to set self.dir_path and make the function use it, or pass it to the function as a parameter? (_files_from_rgx function)

And if I want to write unit tests, should I test write a test for the function and another for the constructor, or write tests for the constructor, which implicitly will enter the function which I have done in the test above?

There's a couple of things to consider when designing a class -> test relationship.

First, I'd try hard to avoid the situation you're setting up where a command class can crash in its own constructor. It's usually a better idea to make sure that constructors are exception-safe if possible, since you never know what calling code may want to do with them. So the first thing I'd consider is simply turning _extract_files() into a regular public method and not calling it in the constructor.  Among other things, it simplifies the test code by allowing you to create test objects without pre-configuring things -- but it's also better in general.

Which leads to number 2: it's better not to rely on the state of a local hard drive in tests. Ideally a test suite runs in a completely reproducible, zero-entropy environment which never changes  -- not even if your build machine is low on hard drive space, the network connection goes down, a user doesn't actually have a writable tempdir, and so on.  These kinds of conditions are often hard to work around, but thinking them through tends to make for better testing  - and that, in turn, leads to better code.

In your case, _files_from_rgx really wants to be operating on the state of a local directory.  However the 'work' is really coming from the way the regex parses the file list.  So, one way to get good coverage is to refactor Command so that the parsing logic works on a file list (or better yet, any iterable since that's more generous).  In this case if you took the regex-filter function out you might get something like this for a start:

def _files_from_rgx(self, rgx):
    """
    Looks for files in the directory `self.dir_path` with names
    matched to the provided regex and returns those files names.
    """
    f = []
    # incidentally since you're not using the recursion here,
    # you could probably replace this with
    # f = os.listdir(self.dir_path)

    for (dir_path, dirnames, filenames) in os.walk(self.dir_path):
        f.extend(filenames)
        break

    return list(self._filter_by_regex(rgx, f))

def _filter_by_regex(self, rgx, inputs):
    """
    loop over <inputs> yielding out values which are full-name
    matches for the regex <rgx>
    """
    compiled_rgx = re.compile(rgx)

    for each_file in inputs:
        matched = compiled_rgx.match(each_file)
        if matched and matched.span()[1] == len(each_file):
            yield each_file

Now you can test the _filter_by_regex function in isolation :

  def test_filter_by_regex(self):
       example = Command(some_test_args):
       known_regex = ".*\.py"
       sample_data = ['a.py', 'b.py', 'c.cpp', 'd.exe', 'e.py']
       results = list(example._test_filter_by_regex(known_regex, sample_data))
       self.assetListEqual(results, ['a.py', 'b.py', 'e.py']

This doesn't require you to actually create temp files and so on (note that you have to be a good citizen if you do create temp files, because if the tests are running on a build server you don't want to be leaking test files all over the place!).

You didn't include source for _file_from_name() and _names_from_file but it would be a good idea to structure them and their tests so you can test them in the same way, without going to disk.  You'll eventually have a function somewhere what will have to call os.listdir() or os.walk and feed those functions -- but at least the part of the logic that is all yours will be covered by tests.

The test you wrote does one thing that's really great, which is guaranteeing the exception raised by an empty file list.  That's a very good test, because it guarantees predictable behavior for users.  You can make that functionality independently testable with a similar small refactor that moves that into a separate function:

def _validate_file_list(self, file_list):
   if not file_list:
        raise FileNotFoundError("No files match")

And a matching test:

def test_validate_raises_on_empty_file(self):
       example = command(some_test_args)
       should_raise = lambda: example._validate_file_list([])
       self.assertRaises(FileNotFoundError, should_raise)

There's another approach you could take, but I would recommend learning how to do the "old-fashioned" way first.  The mock module would allow you to simulate the state of a disk by replacing calls to things like os.walk with a dummy function that return the same results every time without looking at the disk.  It's a very  powerful technique, but it can encourage code that's otherwise not easily testable.  I'd figure out more ways to isolate and test key code paths without mock before adding it in as a final level of testing.

For a good look at the philosophy of breaking out logic and IO, I highly recommend this talk by Python core dev Brandon Rhodes:  https://youtu.be/PBQN62oUnN8


Generating a range of numbers For practice I wrote a function to generate a list of numbers from a start to an end by a step. I couldn't make it- like the built-in range- default start to 0, because all optional parameters must come after required ones, so def range(start=0, end, step=1) won't work.

Here's my script:

def Range(start, end, step=1):
    """Returns a list of numbers beginning from start and
       incremented by step to end (with end excluded).

      start: an integer specifies the start of the range.

      end: an integer specifies the end of the range.

      step: an integer specifies the number to increment by.
      """

    num = start
    result = []

    while num < end:
        result.append(num)
        num += step

    return result

How can it be refactored and optimized ?

Notes:

I'm a hobbyist and beginner (in Python in particular, and programming in general).

I'm not sure how the actual python implementation for this works but you could query the length of *args to assign the parameters to the right variables:

def Range(*args):
    len_args = len(args)
    if len_args == 2:
        args = args + (1,)
    elif len_args == 1:
        args = (0, args[0], 1)
    start, end, step = args
    ...

In addition I would make this a generator (similar to how it is in python 3.x):

def Range(*args):
    len_args = len(args)
    if len_args == 2:
        args = args + (1,)
    elif len_args == 1:
        args = (0, args[0], 1)
    start, end, step = args

    while start < end:
        yield start
        start += step


Printing all the permutations of a string in alphabetical order It is a code eval challenge question https://www.codeeval.com/open_challenges/14/.
My answer was accepted, and I modified my code again to improve it. Please let me know is it possible to improve it more.

#include <iostream>
#include<fstream>
#include<algorithm>
#include<string>

void create_permute( std::string &record )
{
    if( !record.empty() )
    {
        std::sort( record.begin(), record.end() );
        bool flag=0;
        do{
            if( flag )
            {
                std::cout<<",";
            }
            flag = 1;
            std::cout << record;
        } while( std::next_permutation( record.begin(), record.end() ) );
        std::cout << "\n";
    }
}
void readInputFile( std::string filename )
{

    std::ifstream infile;
    infile.open( filename );
    std::string record;
    while( std::getline( infile,record ) )
    {
       create_permute( record );
    }
}

int main( int argc, char* argv[] )
{
    if( argc < 2 )
    {
        std::cout << "usage: filesize filename" << "\n";
        exit( 0 );
    }
    std::ios_base::sync_with_stdio( false );
    readInputFile( argv[ 1 ] );

    return 0;
}

A boolean tested each time, and firing just once on the first iteration is unaesthetic (and a waste of time). Consider instead

    std::cout << record;
    while (std::next_permutation(record.begin(), record.end())) {
        std::cout << ',' << record;
    }
    std::cout << '\n';

Otherwise looks good. Congratulations for not using namespace std.


Moving files and leaving links in bash The objective of this code is to create a command that move files and leaves links in their place. I'm just a beginner in shell scripting and it would be quite useful to hear your opinions on how to improve my code.

I already used https://www.shellcheck.net/ to check my code.

Questions

I change directory several times because I was creating broken links with relative paths. Is there a better way to do this?

Is there a better way to get the path of the files than to use cut with wc and using cd and pwd?

mvln.sh

#!/bin/bash
if [ "$#" -lt 2 ];then
    echo -e "mvln: Move file and leave a link in its place\\nUse: mvln [TARGET] [DIRECTORY]";
    exit 1;
fi

pathTo=$(cut -d ' ' -f "$#" <<< "$*");
if ! [ -d "$pathTo" ]; then
    echo "mvln: $pathTo is not a directory";
    exit 1;
fi

pathOriginal=$(pwd);
cd "$pathTo" || exit 1;
pathTo=$(pwd);
cd "$pathOriginal" || exit 1;

files=$(cut -d ' ' -f -$(("$#" - 1)) <<< "$*");

for i in $files;
do
    if [ "$i" -ef "$pathTo" ]; then
        echo "mvln: '$pathFrom' and '$pathTo' are the same file";
        continue;
    fi
    wordCount=$(tr -t '/' ' ' <<< "$i" | wc -w);
    pathFrom=$(cut -d '/' -f -$((wordCount - 1)) <<< "$i");
    file=$(cut -d '/' -f "$wordCount" <<< "$i");
    mv "$i" "$pathTo" || continue;
    cd "$pathFrom" || exit 1;
    ln -s "$pathTo/$file" ".";
    cd "$pathOriginal" || exit 1
done;
exit 0;

Creating test files

mkdir ~/Documentos ~/Scripts ~/Scripts/test ~/Scripts/test/ln
touch ~/arqHome.txt ~/Documentos/arqDoc.txt ~/Scripts/arqScripts.txt ~/Scripts/test/arq1.txt ~/Scripts/test/arq2.txt ~/Scripts/test/arq3.txt ~/Scripts/test/arq4.txt ~/Scripts/test/arq5.txt

Examples of how the command works:

Example1

Move all files from 1 place

bash mvln.sh ~/Scripts/test/arq* ~/Scripts/test/ln

Example2

Move files from several places

bash mvln.sh ~/arqHome.txt ~/Documentos/arqDoc.txt ~/Scripts/arqScripts.txt ~/Scripts/test/arq* ~/Scripts/test/ln

Instead of temporarily changing directories, you can call realpath to get the absolute path to a directory. There is also readlink which does similar job.

To generate the files array:

files=("$@"); unset "files[-1]"

The rest looks okay to me. However, keep your usage of ; consistent. If you are using it, put them in each statement; otherwise skip it altogether.


Coupon system optimization I have the following code for my coupon system. It should work but I'm sure I can optimize it.

Any suggestions would be welcome.

@price_to_pay = @booking_request.guests * @table_offer.price_cents / 100
@remaining = @coupon.current_amount - @price_to_pay
if @remaining > 0
  @coupon.current_amount = @remaining
  @price_to_pay = 0
elsif @remaining = 0
  @coupon.current_amount = 0
  @price_to_pay = 0
elsif @remaining < 0
  @coupon.current_amount = @remaining
  @price_to_pay = @remaining * -1
end
coupon.save

I think this replicates the logic:

price = @booking_request.guests * @table_offer.price_cents / 100
@remaining = @coupon.current_amount - price
@coupon.current_amount = @remaining
@price_to_play = @remaining >= 0 ? 0 : -@remaining
coupon.save

Note that I avoid setting @price_to_play to a value and changing it afterwards, that kind of variable re-using makes code harder to understand.


Fetch live data from SQLite I'm writing a small Python script which needs to fetch live data from a database (I'm using SQLite 3 for now). I may reuse this function for a lot of scripts. Can this code be made better?

def fetch_database(database, table, filter_dict, case=None):
    """
    Takes filter parameter as a dict
    dict = {'column_name':['filter', 'filter2'], 'column_name2'=[], ..}
    case = wrap around the dictionary key(UPPER, LOWER, etc..), None
    database (with pathname ex: database.db
    table
    column = dict.keys()
    """

    conn = sqlite3.connect(database)
    c = conn.cursor()
    filter_dict = dict_format(filter_dict) # A separate function deletes empty keys from dictionary
    keys_list = filter_dict.keys()
    statement = 'SELECT * FROM ' + table
    if len(keys_list) > 0:
        statement += ' WHERE '
        for keys in keys_list:
            if case == None:
                key = keys
            else:
                key = case + '(' + keys + ')'
            temp_data = filter_dict[keys]
            temp_size = len(temp_data)
            if keys_list.index(keys) != 0:
                statement += ' AND '
            if temp_size > 0:
                for data in temp_data:
                    if temp_data.index(data) == 0:
                        statement += key + '="' + data + '"'
                    else:
                        statement += ' OR ' + key + '="' + data + '"'
    dataset = c.execute(statement)
    dataset = dataset.fetchall()
    return dataset

I'm surprised nobody mentioned the code being vulnerable to SQL injection attacks:

(XKCD source)

Using string formatting and string concatenation to construct SQL queries is, first of all, unsafe and also, fragile - you will have to handle the argument types, balancing and escaping quotes yourself manually.

Instead, you should parameterize your queries - this way, you are not only protected from SQL injections, but also let the database driver worry about the python-to-database type conversions, sample:

query = """
    SELECT column1
    FROM table
    WHERE column2 = ?"""
cursor.execute(query, (data, ))

Note how parameters are passed in a separate argument to execute().

On the other hand, it feels like you may be reinventing the wheel in a certain way - please check if switching to an ORM like SQLAlchemy or peewee would help to avoid re-writing boilerplate code.


Prompting for two integers and dividing, using exceptions to handle divide-by-zero errors I'm trying to learn exception handling in C++.

I wanted to read two integers and divide them and print them.

The code should throw an exception when the second integer is zero, ask the user to re-enter the second integer, and then complete the divide operation.

I coded something like this:

#include <iostream>
#include <stdexcept>

using std::cin;
using std::cout;
using std::endl;

int main() {
    int i1, i2;
    cout << "Give two integers" << endl;
    cin >> i1;
    try {
        first:
        cin >> i2;
        if (i2 == 0)
            throw 0;
        else
            cout << i1 / i2 << endl;
    }
    catch (...) {
        cout << "Second integer cannot be zero , please enter a valid integer" << endl;
        goto first;
    }
    system("pause");
    return 0;
}

However, I was advised not to use goto at all costs.

Can anyone give me some alternative way?

Exceptions are for exceptional cases

  I'm trying to learn exception handling in C++.

This is a very bad example to learn exceptions. They are meant for situations where you cannot know that there is something wrong. For example if you want to use std::vector<int> but you're out of memory:

std::vector<int> vec(4000000000LL); // will likely throw std::bad_alloc

I'll add an exercise for you at the end of this review. But let's have a look at your code.

A review of your code

You're not using using namespace std, which is a great plus.

However, you include <stdexcept>, which isn't necessary here. You don't use any of the standard exception, instead, you throw 0. However, system is in <cstdlib>. You should include that one instead.

Next, naming.

int i1, i2;

Those names don't have any meaning. What is i1? What is i2? Naming is hard, but it's important. You're going to use them as numerator and denominator, or as dividend and divisor, so call them appropriately:

int numerator, denominator;

Getting rid of goto

Now to your try block. Don't. Use. goto. You want to repeat the block until you didn't get 0. We can do that with a simple while:

int main (){
    int numerator, denominator;

    cout << "Give two integers" << endl;
    cin >> numerator;

    // continue forever                               (1)
    while(true) {
        try {
            cin >> denominator;
            if (denominator == 0)
                throw 0; // this will "go to" catch   (2)

            cout << numerator / denominator << endl;
            break; // we didn't throw, we can stop    (3)
        }
        catch (...) {
            cout << "Second integer cannot be zero , please enter a valid integer" << endl;
        }
    }

    system("pause");
    return 0;
}

A "clean" version without exceptions

However, that's not how I would write the program, since exceptions are meant for exceptional cases. I would write

#include <iostream>
#include <cstdlib>

int main() {
    int numerator, denominator;

    std::cout << "Please enter two integers." << std::endl;
    std::cin >> numerator >> denominator;

    while(denominator == 0) {
        std::cout << "Second integer cannot be zero. Try again." << std::endl;
        std::cin >> denominator;
    }

    std::cout << (numerator / denominator) << std::endl;

    system("pause");
    return 0;
}

As you can see, exceptions aren't really necessary to keep the user from entering a zero. Furthermore, the control flow is a lot easier to read.

Note that system("pause"); won't work on Linux, since there is no application called pause. If possible, try to use your IDEs features to keep your console window alive even after your program finished, or have the output logged somewhere, or use it in the CMD/PowerShell.

Exception exercise

We need a better exercise for your exceptions. How about this?

int divide(int numerator, int denominator) {
    if(denominator == 0)
        throw "divide: division by zero";
    return numerator / denominator;
}

This is a valid place for an exception. A function has only a single return value, so it can only tell you that something is wrong with an exception*.

Exercise: Try to use that function in your code and handle the exception. Also show the exception to the user, they're probably interested in what went wrong.

You should use the code similar to

std::cout << divide(numerator, denominator) << std::endl;

that is you don't check the denominator. Note that this is still a contrived example.

* Technically, that's not true. You can use references, pointers, or wrap the return value in some struct/variant, but let's keep things simple


Print every character in a line if it's greater than 80 characters How can I improve this program?

#include <stdio.h>
#define MIN 80

int main(void)
{
    char line[MIN];
    int i, c;

    for (i = 0; (c = getchar()) != EOF && c != '\n'; ++i)
    {
        if (i < MIN - 1)
            line[i] = c;
        else
        {
            if (i == MIN - 1)
                printf("%s", line);

            printf("%c", c);
        }

    }

    return 0;
}

A few notes (overall very good for a beginner):

Change MIN so that it's a static const because it respects scope and is type-safe.  Also, what is it the minimum for?  The variable name could be more specific.
Very nice on the int main(void)!
Always initialize your variables when you declare them.  This helps prevent unintended behavior later on in the program.
Declare i within your for loop, this should be standard by now (and has been as of C99).

for (int i = 0; ...)

You could also do this with your c variable, but I wouldn't recommend that as i is the counter that is tied directly to that for loop.
The rest of your for loop may be hard for some beginners to read, but I actually like it the way it is and don't find it too hard to read.
The way you have your if statement may be of concern to some.

if (i < MIN - 1)
    line[i] = c;

Their argument is that it could lead to potential bugs in the future if maintained incorrectly.  I'm going to say it's fine, as long as you're consistent with it AND use GCC 6's -Wmisleading-indentation flag to help catch those problems with it in the future
You don't append \0 to line when you are about to print it.

if (i == MIN - 1)
{
    line[i] = '\0';
    printf("%s", line);
}

You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard  knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a
  value of 0.

Final code:

#include <stdio.h>

static const int MIN_LENGTH = 80;

int main(void)
{
    char line[MIN_LENGTH];
    int c = 0;

    for (int i = 0; (c = getchar()) != EOF && c != '\n'; ++i)
    {
        if (i < MIN_LENGTH - 1) line[i] = c;
        else
        {
            if (i == MIN_LENGTH- 1)
            {
                line[i] = '\0';
                printf("%s", line);
            }

            printf("%c", c);
        }
    }
}