Reading values from a file and assigning to another class I'm assigning values to an object by passing it to another class. Basically, I'm reading values from a file and assigning it by passing it to another class.

This is the POJO class for the details in a file.

public class FileDetails
{

    String URL;
    String Port;
    String UserName;
    String Pwd;

    public String getUserName() {
        return UserName;
    }

    public String getPort() {
        return Port;
    }
    public String getPwd() {
        return Pwd;
    }
    public String getURL() {
        return URL;
    }
    public void setPort(String port) {
        this.Port = port;
    }
    public void setPwd(String pwd) {
        this.Pwd = pwd;
    }
    public void setURL(String URL) {
        this.URL = URL;
    }
    public void setUserName(String userName) {
        this.UserName = userName;
    }
}

This is the class which I am using to set the values into that an instance of FileDetails.

public class FileService extends FileDetails
{
    public FileService(FileDetails fileDetails)
    {
        FileDetails fileDet = null;

        ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.configure(DeserializationConfig.Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
        try
        {
            fileDet = objectMapper.readValue(new File("res.json"), fileDetails.class);
        }
        catch(IOException e)
        {
            e.printStackTrace();
        }
        if (fileDet != null) {
            fileDetails.setUserName(fileDet.getUserName());
            fileDetails.setPwd(fileDet.getPwd());
            fileDetails.setURL(fileDet.getURL());
            fileDetails.setPort(fileDet.getPort());
        } else {
            System.out.println("Didn't set details");
        }
    }
}

This works. Everywhere I call this code, I am using two lines of code, like

FileDetails fileDetails = new FileDetails();
FileService fileService = new FileService(fileDetails);

I want to know if this is the optimum way to get things done, or if there are any better ways to do this.

Convention violations

Java conventions generally impose "egyptian bracing style", with the opening brace on the same line as the block start. Yes, also for classes.

Java convention governs that fields should be named camelCase. This means that FileDetails should have the fields url, port, userName and password

It's convention to restrict the visibility of fields as much as possible. All fields in FileDetails should be private.

Constructor responsibilities
So... let me get this straight... You're using a constructor of a separate object to set the properties of an object you pass into the constructor, that is of a supertype and ...
This is wrong in more ways than I imagined one could get constructors wrong ...
Let me break down, what constructors are for:

Constructors construct objects
Constructors construct objects
NOTHING ELSE

What the FileService constructor does is the following:

It takes a FileDetails object.
It loads a FileDetails object from a (hardcoded) json file.
It copies over the loaded object's values to the passed object.

At no point it does initialize the object it's responsible for. Neither does it perform validation or any other useful constructor things.
What this constructor basically is is called "factory method". You're using the constructor of a separate class to initialize the properties of a class you control.
Where did the separate class come from???
Instead of doing it this way, it would make much more sense to have the FileDetails constructor be responsible for the work you do there...
Consider the following:
public class FileDetails {
    private String url;
    private String port;
    private String userName;
    private String password;

    public FileDetails(String url, String port, String userName, String password) {
         this.url = url;
         this.port = port;
         this.userName = userName;
         this.password = password;
    }

    public FileDetails(Path detailsFile) {
         try (InputStream in = Files.newInputStream(detailsFile, StandardOpenOption.READ)) {
             ObjectMapper mapper = new ObjectMapper();
             mapper.configure(DeserializationConfig.Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
             FileDetails details = mapper.readValue(in, FileDetails.class);

            this.url = details.url;
            this.port = details.port;
            this.userName = details.userName;
            this.password = details.password;
        } catch (IOException e) {
            e.printStackTrace(System.err);
        }
    }
    // getters and setters omitted
}

Along the side this resolves the following additional problems in the code:

Overlarge scope of objectMapper.
Overuse of getters and setters inside a class.
Missing lifetime control of Resources.
Hardcoded path value for the file with resources in it.

Now here comes the fun part. The second constructor I wrote is really unnecessary. It's a "factory method" (as I mentioned above). What that means is, it can be rewritten like so:
public static FileDetails fromFile(Path detailsFile) {
    try (InputStream in = Files.newInputStream(detailsFile, StandardOpenOption.READ)) {
        ObjectMapper mapper = new ObjectMapper();
        mapper.configure(DeserializationConfig.Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
        return mapper.readValue(in, FileDetails.class);
    } catch (IOException e) {
        throw new UncheckedIOException(e);
    }
}


How should I pass settings to a module in Perl that also exports symbols? In Test::Version I want to add configuration settings that allow one to toggle certain features. I'm thinking about passing an anonymous hash to a custom import like so.

use Test::Version 1.2 qw( version_ok ), { is_strict => '1', has_version => '0' };

other options I've thought of include package variables (though more ugly in my opinion).

use Test::Version 1.2 qw( version_ok );
$Test::Version::IS_STRICT   = 1;
$Test::Version::HAS_VERSION = 0;

or maybe using some other kind of operator to denote config settings on import, though I'm not sure exactly how to implement this.

use Test::Version 1.2 qw( version_ok ), +is_strict => '1', +has_version => '0';

Keep in mind that this has a simple functional interface, so passing objects, or using config files is overkill. Any preferences? or other suggestions?

I would stay away from changing the normal workings of import(). I've never seen a module mix exporting and configuration at the same time (maybe you have, though), so leave those out (options 1 and 3). I think you are right in thinking that option 2 isn't too pretty. That leaves us with none of those options.

I think instead of this you should just allow calling version_ok and version_all_ok with the options hash.

version_ok({ is_string => 1, has_version => 0 });

Clear and standard, nothing odd going on.  From the way it seems like the module is going to be used this isn't a burden on the user (since there will normally only be one call to this in the entire test suite).


Ruby - Sending commands to multiple servers using Net::SSH The idea

At my work, I'm tasked with retrieving logs for a list of online orders from a total of 12 servers. To do this, I SSH into them (with a jump host in between) one by one, run the script for every order, then parse the output. Overall, a very tedious process that I'm sure can be automated.

W -> J -> S

W = My Windows VM

J = Red Hat jump box

S = The Red Hat production servers

So the basic idea is the following:

Connect to every server by SSH
On every server, run the commands for all orders which are passed by command line
Associate each output with the order

Here is what I've come up with so far, and it is working on my home lab. But before I bring it in to work to propose it and try it on production servers, I want to know how the code can be cleaned up or done more efficiently.

The code

require 'net/ssh'
require 'io/console'
require 'pp'

ORDERS = ARGV.dup()
ARGV.clear()

SERVERS = [
    "prod1",
    "prod2",
    "prod3"
]

COMMAND = "aux_search.sh"

CONFIG = "C:/Users/myuser/.ssh/config"

RESULTS = ORDERS.each_with_object({}) { |k, v| v[k] = "" }

puts("Enter your username: ")
USERNAME = gets().chomp()
puts("Enter your password for accessing the servers: ")
SERVERPASSWORD = STDIN.noecho(&:gets).chomp()
puts("Enter your sudo password: ")
SUDOPASSWORD = STDIN.noecho(&:gets).chomp()

SESSIONS = []
SERVERS.each do |server|
    session = Net::SSH.start(server, USERNAME, :password => SERVERPASSWORD, :config => CONFIG,
        :verbose => :debug, :auth_methods => ["publickey", "password"], :verify_host_key => :accept_new)

    SESSIONS << session
end

SESSIONS.each do |ssh|
    ORDERS.each do |order|
        ssh.open_channel() do |channel|
            channel.on_data do |_, data|
                RESULTS[order].concat(data)
                if data.match(/\[sudo\]/)
                    channel.send_data("#{SUDOPASSWORD}\n")
                end
            end

            channel.request_pty() do |_, success|
                raise "Failed to request TTY for order #{order}" unless success
            end

            channel.exec("sudo ./#{COMMAND} #{order}") do |_, success|
                raise "Could not execute command #{COMMAND} #{order}" unless success
            end
        end
    end
    ssh.loop()
end

puts("===Results===")
pp(RESULTS)

My ssh config

Host jumpbox
    HostName 192.168.16.2
    User johrus2
    IdentityFile C:\Users\myuser\.ssh\id_rsa3
    Port 22

Host prod1
    HostName 192.168.16.3
    User johrus2
    IdentityFile C:\Users\myuser\.ssh\id_rsa3
    ProxyJump jumpbox
    Port 22

Host prod2
    HostName 192.168.16.4
    User johrus2
    IdentityFile C:\Users\myuser\.ssh\id_rsa3
    ProxyJump jumpbox
    Port 22

Host prod3
    HostName 192.168.16.5
    User johrus2
    IdentityFile C:\Users\myuser\.ssh\id_rsa3
    ProxyJump jumpbox
    Port 22

```

I think this is interesting code. I admit I haven't dug much into Ruby's Net::SSH library, so I bet you've learned some things about it in writing this code. It sounds to me like your biggest concern now is about professionalism, stability, and dependability which is why you want feedback before offering to use this code in a production environment. If so, I feel like you're asking all the right questions.

Here are my suggestions:

Search for prior art
Don't touch passwords if you can help it, and other security practices
Add some tests

Prior Art

You haven't mentioned yet if you've looked into existing tools that have already done this work. For example, you could use Capistrano, which is written in Ruby and designed for the type of scenario you're describing. Capistrano is fairly well-maintained. You could put it on your jump box, if you wanted. If Capistrano looks too cumbersome for what you're doing, you may want to use its sshkit library, which handles some of the ssh concerns you're navigating manually here.

Security Practices

I see you have public/private keypairs for every server you're connecting to. Try to use them instead of getting user input for a password. I would lean on the side of requiring them for anyone using a tool like this. In fact, I might have a distinct identity for automation so that I can revoke or recreate its keys without affecting any other way I can log in to the system. You may also want to add logging so you can audit your script to make sure it did what you intended. This is also helpful as a changelog to look back on when your script worked fine and you're troubleshooting unexpected side-effects of the commands it ran.

The nice thing about this specific identity is you can also leverage it to avoid passing sudo passwords around. This identity could have a limited list of commands it can run either in your sudoers file or without needing sudo. For example, I have used Capistrano to log in as a user that does not have root permissions, but does have elevated permissions that my basic account does not have. This elevated user had file permissions so it could update the code for the servers' websites without requiring sudo, though it couldn't do too much else.

More generally, I get nervous whenever I'm writing any code that handles user input for security credentials. I'd say that if there's already an existing project where flaws in handling security are likely to result in the announcement of a public CVE, it's better to use those projects for than writing your own security implementations to use in production.

In summary

use something more likely to get security audits than you are
have a layered plan for what you do if something goes wrong or the system is abused

Add Some Tests

Do you know what errors SSH is likely to raise if something goes wrong? Should that stop the whole process without continuing? Does your code do that in a way you're happy with?

These are the kind of answers that tests are good at answering for you. One thing I like to keep in mind when writing separate test code feels like a waste of time is the saying, "Everyone tests their code. Some people then throw the tests away, and some people keep them."

I suspect, looking at your code, it would be hard to stub out the ssh library to simulate what happens when when several commands succeed and then one fails. (A discussion of how to stub and mock that sort of thing in Ruby tests feels to big to handle in this answer. Let me know if you have trouble finding guides for it online.) The fact that writing test you'd like to have feels difficult is an excellent sign that you should split up your code differently from how you've written it.

As an example, you could have one chunk of code that accepts some generic connection management object, and then worries about the sequencing of commands and handling failed commands. Then you could have another chunk of code that is the connection object, abstracting the SSH library down to the few parts of it you care about for this script. Maybe there's an even better way to make that split that you'll find once you sit down to it.

Either way, once you reorganize your code so the tests are easier to write, you'll also find it gets easier to make changes, fix bugs, add new features even if you were to never run the tests again. (But you should run the tests again — you already wrote them, and they're just sitting there. No need to waste the effort.)

Good Luck Out There

I hope this helps. Feel free to ask me for further clarification on any of this if you want.


Expanding branches of a family tree I'm making a family tree. The following script allows the user to expand branches of the tree. It works fine, but there must be a better way.

You can see it working here.

$(function(){
    $('.oc').prepend('')
    $("ul[id|=Nested]").hide();
    $("a[id|=trigger_Nested]").click(function() {
        var text = $(this).siblings("ul");
        if (text.is(':hidden')) {
            text.slideDown("fast");
            $(this).next("span").html('');
        } else {
            text.slideUp("fast");
            $(this).next("span").html('');
        }

        return false;
    });
});

A couple notes:

I would use classes instead of ids for the nested links. I don't know if it's faster to search the DOM for all elements with a specific class, but it looks cleaner and doesn't require you to create unique ids for each element:

$('a.nested-trigger').click(...);

I would also use classes for the icons, as it's less expensive than replacing DOM nodes:

HTML:

CSS:

.oc {
  position: relative;
  right: 10px;
  display: inline-block;
  width: 16px;
  height: 16px;
}
.collapsed {
  background: url(icons/bullet_add.png);
}
.expanded {
  background: url(icons/bullet_take.png);
}

JS:

$(this).next("span").removeClass("collapsed").addClass("expanded");

This is related more to the UX of your demo but I would make the icons themselves clickable as well, because they're big enough to look like buttons, and they overlap part of the node.


Do CriteriaQuery code more readable and elegant I wrote the code and would like to change it to more readable and elegant:

@Repository
public class ProductRepositoryImpl implements ProductCustomRepository {

private static final int MORE_THEN_RESULT = 1;

@PersistenceContext
private EntityManager em;

public List<Product> findByFilter(Filter filter) {
    final CriteriaBuilder cb = em.getCriteriaBuilder();
    final CriteriaQuery<Product> cq = cb.createQuery(Product.class);
    final Root<Product> from = cq.from(Product.class);

    try {
        final List<Predicate> predicates = createPredicatesByFilter(cb, from, filter);
        if (!CollectionUtils.isEmpty(predicates)) {
            cq.where(cb.and(predicates.toArray(new Predicate[predicates.size()])));
        }
    } catch (RuntimeException e) {
        return Collections.emptyList();
    }

    cq.select(from);

    return em.createQuery(cq).getResultList();
}

private List<Predicate> createPredicatesByFilter(CriteriaBuilder cb, Root<Product> from, Filter filter) {
    final List<Predicate> predicates = new ArrayList<>();

    if (!StringUtils.isEmpty(filter.getName())) {
        predicates.add(cb.like(cb.lower(from.<String>get("name")), filter.getName().toLowerCase() + "%"));
    }

    final BigDecimal priceBefore = filter.getPriceBefore();
    final BigDecimal priceAfter = filter.getPriceAfter();

    if (priceBefore != null && priceAfter != null) {
        if (priceBefore.compareTo(priceAfter) != MORE_THEN_RESULT) {
            predicates.add(cb.between(from.<BigDecimal>get("price"), priceBefore, priceAfter));
        } else {
            throw new RuntimeException();
        }
    } else if (priceBefore != null) {
        predicates.add(cb.gt(from.<BigDecimal>get("price"), priceBefore));
    } else if (priceAfter != null) {
        predicates.add(cb.lt(from.<BigDecimal>get("price"), priceAfter));
    }

    if (!StringUtils.isEmpty(filter.getCategory())) {
        predicates.add(cb.like(cb.lower(from.<Category>get("category").<String>get("name")), filter.getCategory().toLowerCase() + "%"));
    }

    return predicates;
}
}

In general this looks okay to me.  In fact I have a similar pattern in
an application with lots of optional filters.  Newer Java 8 features
which can make this a bit nicer (e.g. using Optional and method
references).

Throwing/catching RuntimeException like that looks iffy.  At least
add a message to the exception so the reader knows how to interpret
this.
Returning a List only to immediately convert it to an array doesn't
sound that useful, then again, if it's being used from other places it
might be okay.  List also has isEmpty, better than having to call
yet another static function for it.
CriteriaQuery.where
is already using the conjunction, no need to add the and there.
Note that the array can be passed for the
varargs arguments:

if (!CollectionUtils.isEmpty(predicates)) {
    cq.where(predicates.toArray(new Predicate[predicates.size()]));
}

For the the string matching queries additional helper methods might be
nice:

private static startsWithLower(Expression<String> expression, String pattern) {
    return cb.like(cb.lower(expression), pattern.toLowerCase() + "%");
}


Parent with N-Childs where each child can have N-Childs I know my question sounds strange but I have a situation in which I have Parent Data  which can have 'N' Childs and each child can further have N-Childs and so on.

So its Like.

Parent

a. Child

    i. Child
    ii. Child

b. Child

    i.Child

I hope I've made myself Clear. So here is a snapshot of the Database Table:

So I have wrote this Code to get my Data From Database. It is a recursive Function:

public static List<CaseCommunication> GetCaseCommunicationByCustomerCaseId(int customerCaseId)
        {
            var parentCaseCommunications =
                EntityHandler.GetEntitiesContext().CaseCommunications.Where(x => x.CustomerCaseId == customerCaseId && x.ParentCaseCommunicationId == null);

            var mappedParentCommuncations = Mapper.Map<IEnumerable<CaseCommunication>>(parentCaseCommunications).ToList();

            foreach (var parent in mappedParentCommuncations)
            {
                parent.ChildCommunications = GetListOfChildCommunicationsByParentId(parent.CaseCommunicationId);
            }

            return mappedParentCommuncations;
        }

        public static List<CaseCommunication> GetListOfChildCommunicationsByParentId(int caseCommunicationId)
        {
            var childCommunications =
                EntityHandler.GetEntitiesContext().CaseCommunications.Where(x => x.ParentCaseCommunicationId == caseCommunicationId);

            var mappedChild = Mapper.Map<IEnumerable<CaseCommunication>>(childCommunications).ToList();

            foreach (var child in mappedChild)
            {
                child.ChildCommunications = GetListOfChildCommunicationsByParentId(child.CaseCommunicationId);
            }

            return mappedChild;
        }

This is the function called from the other class: GetCaseCommunicationByCustomerCaseId

While GetListOfChildCommunicationsByParentId  is the recursive function.

I was just need to know is there a better way of writing this?

Is this efficient?

It's not strange at all. It's just a tree, or graph, which is very common.

You don't even need two methods: a single one is sufficient.  You should define a single recursive method that takes in a list (or some other collection) of id's and returns those id's and all their  children and grand-children, etc.  If you want the children of a single element instead of a list of id's, you can just pass a list of one element (or define a method that does that).

Actually, it might be better to use a set instead of list of id's as the argument to the method since it would then handle duplicates and circular references.  ALso, if you use a list instead, you would have to specify in the documentation what the ordering of the returned list means.


Verifying if two numbers are equal using ArrayLists I have some question about the code below. It works correctly, but:

Is there a better/another way to solve the exercise (as an expert would have done:) or mine it's perfectly done?
Also, I don't see the point of using ArrayList instead of a normal vector like int a[].
In the method analysis I wanted to set as parameters (ArrayList a,ArrayList b) but an error (incompatible types List cannot be converted to ArrayList) appears in this int a = Testing.analysis(seq1, seq2, t1); line. I thought  I was working with ArrayList since I instantiated my sequences as such.

Exercise:

Perform a program that uses a function that returns a 1 if two very large numbers (digits greater than 5 and where each digit is an element of the arrayList) are equal and a 0 otherwise.
    Example 1:
    Entry
    Number 1: 2 3 4 5 6 7
    Number 2: 1 4 6 8 9 0
    Output "They are not equal numbers"

class Testing {

static int analysis(List a, List b, int t) throws Exception { //using
//ArrayList instead of List marks an error, why?

    int c = 0;

    if (a.size() == b.size() && a.size() > 5) {

        for (int i = 0; i < t; i++) {
            if (a.get(i) == b.get(i)) {
                c++;
            }
        }
    } else {
        System.out.println("oops! the lengths are not the same or some
    of the sequence is less than 6");
        throw new Exception(); //to finish the method
    }

    if (c == t) {
        return 1;
    } else {
        return 0;
    }

}
}

public class Tarea24agoENGLISH {

public static void main(String[] args) throws Exception {
    Scanner q = new Scanner(System.in);

    List seq1 = new ArrayList(); //I wanted to write this List <int> seq=new ArrayList<int>(); but it marked an error, why?
    System.out.println("Introduce the length of the sequence1 ");
    int t1 = q.nextInt();

    for (int i = 0; i < t1; i++) {
        System.out.println("Introduce the digits for sequence1");
        int n = q.nextInt();
        seq1.add(n);
    }

    List seq2 = new ArrayList();
    System.out.println("Introduce the length of the sequence2");
    int t2 = q.nextInt();

    for (int i = 0; i < t2; i++) {
        System.out.println("Introdce the digits for sequence2");
        int n = q.nextInt();
        seq2.add(n);
    }

    int a = Testing.analysis(seq1, seq2, t1);
    if (a == 1) {
        System.out.println("same numbers");
    } else {
        System.out.println("\n different numbers");
    }

}

}

First thing I noticed don't count how many digits are equal, soon as 1 digit is unequal return false, if the loop finishes return true.  Also it seems to me if the sizes are unequal the proper response would be false not an error.


It seems that your allFipsRM function takes as input a data variable name and rolling mean length and outputs information about the rolling mean with the indicated length, computed for each fips value separately.

I see two key issues with the code as currently written:

In two places you are growing objects element by element. Please see Circle 2 of The R Inferno for why this is an inefficient way to grow objects.
A vectorized rolling mean function should be much more efficient than one you code on your own with a loop in R.

The following code fixes these two issues by combining the data for all fips codes in a single call to rbind and by using a vectorized rolling mean function from the RcppRoll package. I did a few other cleanup tasks: passing the data to the function to make it more flexible and using paste0 instead of paste with sep="".

library(RcppRoll)
allFipsRM2 = function(dat, varName, len){
  y <- do.call(rbind, lapply(split(dat, dat$fips), function(x) {
    data.frame(fips=x$fips[1], rm=roll_mean(x[,varName], len), year=seq_len(nrow(x)-len+1)-1)
  }))
  colnames(y)[2] <- paste0("rm",len)
  y
}

We can confirm that this given the same merged results as the original code:

rm1b = allFipsRM2(dat, "x",1)
rm2b = allFipsRM2(dat, "x",2)
outdat2 = merge( dat[, c("fips","x","year")], rm1b, by=c("fips","year") )
outdat2 = merge( outdat2, rm2b, by=c("fips","year") )
all.equal(outdat, outdat2)
# [1] TRUE

To see the performance impact, let's run on a modestly larger version of dat, with 66k rows:

dat <- dat[rep(seq_len(nrow(dat)), 1000),]
system.time(allFipsRM("x", 2))
#    user  system elapsed
#   5.964   2.511   8.549
system.time(allFipsRM2(dat, "x", 2))
#    user  system elapsed
#   0.109   0.007   0.117

We see a speedup approaching 100x for this test data.

You could also update your function to take a vector of lengths in a pretty straightforward way using sapply:

library(RcppRoll)
allFipsRM3 = function(dat, varName, len){
  do.call(rbind, lapply(split(dat, dat$fips), function(x) {
    all.rm <- as.data.frame(sapply(len, function(l) c(roll_mean(x[,varName], l), rep(NA, l-1))))
    colnames(all.rm) <- paste0("rm", len)
    cbind(data.frame(fips=x$fips[1]), all.rm, data.frame(year=seq_len(nrow(x))-1))
  }))
}
outdat3 <- allFipsRM3(dat, "x", c(1, 2))


"Pill" with thumbnail and link I'm currently trying to display a list of movies with this pill-like elements. The code below works but I guess there is a better way or a way to use elements from the twitter Bootstrap. It also bothers me that the date in the brackets isn't centered with the heading.

Please give me feedback how you like my code. I'm always open for any improvements or ideas :)

.pillLink > h {
 display: inline-block;
}

.pillLink {
 display: inline-block;
 padding-left: 20px;
}

.pill > #cover {
 height: 100px;
 outline: 2px solid white;
}

.pill {

 padding: 7px;
 margin: 10px;
 width: 500px;
 background-color: silver;
 -webkit-border-radius: 7px!important;
 -moz-border-radius: 7px!important;
 border-radius: 7px!important;
 border: 1px solid grey
}

  <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css" integrity="sha384-1q8mTJOASx8j1Au+a5WDVnPi2lkFfwwEAa8hDDdjZlpLegxhjVME1fgjWPGmkzs7" crossorigin="anonymous">
  <link rel="stylesheet" type="text/css" href="pills_style.css">

Interstellar

   (2014)

The id attribute must to be unique per document. So I think that using ID for the cover image is wrong under the assumption that you may have many pills per page with covers.

I believe that using a class cover would be better in this case. You can still use .pill > .cover selector in the same way to only select covers under pills.


Converting from .xlsx to .txt I have code that converts each sheet of a spreadsheet into a .txt file. It works well, however, given the big number of exports (about 90 .txt files), I'd like to seek advice on how to speed it up.

Sub xlsxTotxt()

Dim i As Integer
Dim directory As String
Dim fname As String
Dim xWs As Worksheet
Dim xTextFile As String
Dim rdate As String

directory = ThisWorkbook.Sheets("Macro").Range("D576").Value
rdate = ThisWorkbook.Sheets("Macro").Range("E47").Value
i = 0
Application.ScreenUpdating = False
Application.DisplayAlerts = False
Do While ThisWorkbook.Sheets("Macro").Range("D577").Offset(i).Value <> ""
fname = Sheets("Macro").Range("D577").Offset(i).Value
Workbooks.Open (directory & fname)
For Each xWs In Workbooks(fname).Worksheets
xWs.Copy
xTextFile = directory & rdate & " - " & xWs.name & ".txt"
Application.ActiveWorkbook.SaveAs filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.Close
Next
Workbooks(fname).Close
i = i + 1
Loop
Application.ScreenUpdating = True
Application.DisplayAlerts = True

End Sub

Your code is incredibly fragile

directory = ThisWorkbook.Sheets("Macro").Range("D576").Value
rdate = ThisWorkbook.Sheets("Macro").Range("E47").Value

If a single row/column gets added or deleted or moved, those ranges are going to move and your code is going to fail completely.

If you can, those values should be in a dedicated sheet, not buried below 500 lines of other stuff. If not, name your ranges.

So, let's say you take cell D576 and name it Directory_Path or something similar. Now, rather than

directory = ThisWorkbook.Sheets("Macro").Range("D576").Value

which is incredibly fragile, you can use

directory = ThisWorkbook.Range("Directory_Path").Value

and so long as nobody actually deletes said row/column, that will always point to the right place.

Use With

Rather than re-referencing the same object over and over, you can use a With statement to hold a reference. Like so:

Application.ActiveWorkbook.SaveAs Filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.Close

Becomes

With Application.ActiveWorkbook
    .SaveAs Filename:=xTextFile, FileFormat:=xlText
    .Saved = True
    .Close
End With

much clearer and easier to read.

Handle things in the right order

Specifically your Application.[Settings]. Anything *meta* like that should go right at the start and right at the end of the method(s) it applies to. This allows you to confirm, at a glance, what the internal state of your method is and check that things are set/reset correctly. You should also disable Application.EnableEvents and Application.Calculation for significant extra speed. Like so:

Sub xlsxTotxt()

    With Application
        .ScreenUpdating = False
        .DisplayAlerts = False
        .EnableEvents = False
        .Calculation = xlCalculationManual
    End With

    ...

    Code

    ...

    With Application
        .ScreenUpdating = True
        .DisplayAlerts = True
        .EnableEvents = True
        .Calculation = xlCalculationAutomatic
    End With

End Sub

Use descriptive naming

Code should be written for other people (including future you) to read. Names should be descriptive, unambiguous and concise. In that order. I highly recommend the excellent, classic article on naming by Joel Spolsky. But in short, things should sound like what they are.

xTextFile. What on earth is that? Looking at it I have no idea. If I had to guess, I'd say it's some kind of file object. Oh, it's the filename you want to save your text file under? Why not call it newFilename? Since it's a full filename (including directory path). Maybe newFullfilename or just fullFilename might be even more descriptive.

xWs suffers from the same problem. I can guess it's a worksheet object but beyond that? no idea. Since you just use it to iterate over the sheets in your workbook, maybe just call it currentSheet?

fname. Same thing. filename. Since it's the filename for the workbook you're opening, how about targetWorkbookFilename? Sure, it's long, but screen real estate is cheap, cognitive processing is not and that name's an awful lot easier to understand and work with.

Also, move your declarations near to where they're used. This helps you *see* the different parts of your method and helps you keep track of where you are, refer back to your declarations and see ways to split things up into logical sub-methods. Personally, I prefer to keep declarations outside of Loop structures, but that's personal preference.

Objects are your friend

VBA has a huge, extensive, comprehensive object model for every Office Application. Use it.

ThisWorkbook.Sheets("Macro")

You refer to that sheet multiple times. Rather than continually repeating that reference, put it in a worksheet object and then just refer to the object.

Dim macroSheet As Worksheet
Set macroSheet = ThisWorkbook.Sheets("Macro")

Dim dateString As String
dateString = macroSheet.Range("E47").Value

Same with Workbooks.

Do While ThisWorkbook.Sheets("Macro").Range("D577").Offset(i).Value <> ""
fname = Sheets("Macro").Range("D577").Offset(i).Value
Workbooks.Open (directory & fname)
For Each xWs In Workbooks(fname).Worksheets
xWs.Copy
xTextFile = directory & rdate & " - " & xWs.name & ".txt"
Application.ActiveWorkbook.SaveAs filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.Close

Relying on ActiveWorkbook being the one you want is very fragile. Just make it a proper object and then your references will always be accurate:

Dim targetWorkbook As Workbook
Set targetWorkbook  = Workbooks.Open (directory & fname)

...

For Each currentSheet In targetWorkbook.Sheets()

    ...

    With targetWorkbook
        .SaveAs filename:=xTextFile, FileFormat:=xlText
        .Saved = True
        .Close
    End With

Indenting

Indenting is a wonderful way to let you see the structure of your Sub. It'll let you pick out nested-logic (Loops, Ifs, Fors, Withs etc.) and very quickly get a rough idea for where the important stuff in your sub is. Compare:

Do While ThisWorkbook.Sheets("Macro").Range("D577").Offset(i).Value <> ""
fname = Sheets("Macro").Range("D577").Offset(i).Value
Workbooks.Open (directory & fname)
For Each xWs In Workbooks(fname).Worksheets
xWs.Copy
xTextFile = directory & rdate & " - " & xWs.name & ".txt"
Application.ActiveWorkbook.SaveAs filename:=xTextFile, FileFormat:=xlText
Application.ActiveWorkbook.Saved = True
Application.ActiveWorkbook.Close
Next
Workbooks(fname).Close
i = i + 1
Loop

With

Do While ThisWorkbook.Sheets("Macro").Range("D577").Offset(i).Value <> ""

    fname = Sheets("Macro").Range("D577").Offset(i).Value
    Workbooks.Open (directory & fname)

    For Each xWs In Workbooks(fname).Worksheets

        xWs.Copy
        xTextFile = directory & rdate & " - " & xWs.Name & ".txt"

        With Application.ActiveWorkbook
            .SaveAs Filename:=xTextFile, FileFormat:=xlText
            .Saved = True
            .Close
        End With

    Next

    Workbooks(fname).Close
    i = i + 1

Loop

And suddenly you can see your program flow, and where the real work is happening, and where loops start/end, in just a glance. Adding some whitespace also goes a long way to helping readability.

Putting it all together

Sub xlsxTotxt()

    With Application
        .ScreenUpdating = False
        .DisplayAlerts = False
        .EnableEvents = False
        .Calculation = xlCalculationManual
    End With

    Dim parentDirectoryCell As Range
    Set parentDirectoryCell = ThisWorkbook.Range("Directory_Path")

    Dim parentDirectoryPath As String
    parentDirectoryPath = parentDirectoryCell.Value

    Dim dateString As String
    dateString = ThisWorkbook.Sheets("Macro").Range("E47").Value

    Dim fullFilename As String
    Dim currentSheet As Worksheet
    Dim targetWorkbook As Workbook
    Dim targetWorkbookFilename As String

    Dim rowOffset As Long
    rowOffset = 1

    targetWorkbookFilename = parentDirectoryCell.Offset(rowOffset).Value
    Do While targetWorkbookFilename <> ""

        Set targetWorkbook = Workbooks.Open(parentDirectoryPath & targetWorkbookFilename)

        For Each currentSheet In targetWorkbook.Sheets()

            currentSheet.Copy
            fullFilename = parentDirectoryPath & dateString & " - " & currentSheet.Name & ".txt"

            With targetWorkbook
                .SaveAs Filename:=fullFilename, FileFormat:=xlText
                .Saved = True
                .Close
            End With

        Next currentSheet

        targetWorkbook.Close

        rowOffset = rowOffset + 1
        targetWorkbookFilename = parentDirectoryCell.Offset(rowOffset).Value

    Loop

    With Application
        .ScreenUpdating = True
        .DisplayAlerts = True
        .EnableEvents = True
        .Calculation = xlCalculationAutomatic
    End With

End Sub


Bit pattern to console output This function will draw a card in the console

vector<string> draw_card(const Carta& carta){
//The class Carta has two members, carta.value and carta.seme
//both are integers representing the card value or suit
//the card is represented on 9 lines where the second will be the
//card name and suite, while the middle will show a pattern
/*
  |-----------|
  |Asso picche|
  |           |
  |     *     |
  |    * *    |
  |   *****   |
  |  *     *  |
  | *       * |
  |           |
*/

    //card opening and closing line
    string card_delimiter = "|-----------|";

    // vector representing the card
    vector<string> v;

    v.push_back(card_delimiter);

    // construct the second line, so that it will fit different cards names
    // and suites
    string nome_seme = create_semi(carta.seme);
    string nome_valore = create_nome_valore(carta.valore);
    string init_string = "|           |";

    //characters in the init_string will be subsittuted according to the string
    //nome_seme and then after a space nome_valore
    uint valore_offset = 1;
    uint seme_offset = nome_valore.size() + valore_offset + 1;

    for(uint i = 0; i < init_string.size(); ++i){

        if(i >= valore_offset && i < nome_valore.size() + valore_offset){
            init_string[i] = nome_valore[i - valore_offset];
        }

        if(i >= seme_offset && i <= nome_seme.size() + seme_offset - 1){
            init_string[i] = nome_seme[i - seme_offset];
        }
    }

    // update the main vector holding the string
    v.push_back(init_string);

    // initialize the pattern for the center drawing
    vector<uint16_t> middle_matrix;
    middle_matrix.push_back(0x0000); //0      b0000000000000000
    middle_matrix.push_back(0x0400); //1024   b0000010000000000
    middle_matrix.push_back(0x0A00); //2560   b0000101000000000
    middle_matrix.push_back(0x1F00); //7936   b0001111100000000
    middle_matrix.push_back(0x2080); //8320   b0010000010000000
    middle_matrix.push_back(0x4040); //16448  b0100000001000000
    middle_matrix.push_back(0x0000); //0      b0000000000000000

    // scroll through the vector
    for(uint i = 0; i < middle_matrix.size(); ++i){
        string line = "|";
        // bitshift the mask to find if the character will be a space or a *
        for(uint16_t mask = 0x8000; mask != 0x0010; mask >>= 1){
            if(middle_matrix[i] & mask){ line += "*"; }
            else{ line += " "; }
        }
        line += "|";
        v.push_back(line);
    }

    // close the card
    v.push_back(card_delimiter);

    //print the card
    for(unsigned int i = 0; i < v.size(); ++i){
        cout << v[i] << endl;
    }

    return v;
}

I'm posting this for a couple of questions, first of all I'm pretty new to C++ and I would like to know if you would write the whole procedure differently, maybe using replace functions or functions to map the bit pattern.

Second this version will print only one pattern, what would you suggest to do for the other 13?
What I did now was to take a text file, draw the pattern, realize it could be expressed in binary and now I'm here with this integers.

How would you go to maintain clarity? Shall I read in the pattern from a file?
Declare a static array that contains the numbered patterns?
Where shall I place the function that reads the values? or maybe create a class that reads from a "pattern" file and stores them in some member variable, and call it afterwards?

Would you write the whole procedure differently?

Absolutely. In it's current form, it does far too much stuff and (at least in my opinion) is not really flexible enough. Every time you try to add something (e.g. other card patterns, loading patterns from a file, change the card border, ...) the function will just get more and more bloated.

Actually, I'd implement it in a class CardPrinter. This way, you can transfer state (e.g. card patterns) between calls in a nicely, encapsulated way. Also, I'd split the functionality into multiple member functions, so it's easier to reason about them.

  (paraphrasing:) How would you handle multiple patterns?

For patterns as simple as these, I'd probably not bother to encode them in any special format. Just an array of lines is fine (unless you're programming under heavy memory constraints).

These patterns can be set statically, loaded from somewhere or even generated dynamically. Choose what fits your circumstances best! E.g. loading from a file could allow for different patterns, allowing for customization.

  Example:

struct CardPattern {
    std::vector<std::string> lines;
};

class CardPrinter {
    std::vector<CardPattern> patterns;
    std::string delimiter_left;
    std::string delimiter_right;
    std::string delimiter_top;
    std::string delimiter_bottom;
    size_t internal_width;

public:
    CardPrinter(const std::vector<CardPattern>& pats, size_t w = 13, std::string del_left = "|", std::string del_right = "|", std::string del_top = "-", std::string del_bottom = "-")
            : patterns(pats), delimiter_left(del_left), delimiter_right(del_right), delimiter_top(del_top), delimiter_bottom(del_bottom), width(w - del_left.length() - del_right.length()) {
    }

    void print(const Carta& card, std::ostream& stream = std::cout) {
        stream << get_card_visual(card);
    }
private:
    std::string get_card_title(const Carta& card) {
        auto nome_semi = create_semi(card.seme);
        auto nome_valore = create_nome_valore(card.valore);

        return nome_valore + " " + nome_semi;
    }

    const CardPattern& get_card_pattern(const Carta& card) {
        // assuming pattern exists for that value
        return patterns[card.valore];
    }

    std::string format_line(const std::string& line) {
        // just cut off parts that are too long
        // if too short, fill rest with spaces (could be changed, e.g. center text in that case)
        auto visible_part = line.substr(0, internal_width);
        auto filler = std::string(internal_width - visible_part.length(), ' ');

        return delimiter_left + visible_part + filler + delimiter_right + "\n";
    }

    std::string format_card_pattern(const CardPattern& pattern) {
        auto formatted = std::string("");

        for(auto& line : pattern.lines) {
            formatted += format_line(line);
        }

        return formatted;
    }

    std::string format_top() {
        auto top = std::string();
        while(top.length() < internal_width) {
            top += delimiter_top;
        }

        return format_line(top);
    }

    std::string format_bottom() {
        auto bottom = std::string();
        while(bottom.length() < internal_width) {
            bottom += delimiter_bottom;
        }

        return format_line(bottom);
    }

    std::string get_card_visual(const Carta& card) {
        return format_top() + format_line(get_card_title(card)) + format_card_pattern(get_card_pattern(card)) + format_bottom();
    }
};

// Usage example
auto patterns = /* up to you, e.g. */ load_patterns();
auto printer1 = CardPrinter(patterns);
auto printer2 = CardPrinter(patterns, 15, "::", "::", "=", "=");

auto cards = /* get some cards */;
for(auto& card : cards) {
    printer1.print(card);
    printer2.print(card);
}


Find the Maximum Sum of a Contiguous Subsequence in a List I was given a question during interview, and I decided to code it up and learn the difference way to implement this problem. Find the Maximum Sum of a Contiguous Subsequence in a List. I was wondering if you can code review the different ways of solving this problem.

Given a list consisting of both positive and negative integers, find the maximum sum among all the contiguous subsequences of the input list.
Write a function that takes in a list of integers and returns the maximum sum.

#  Example:   input = [6, -1, 3, 5, -10]
#             output = 13 (6 + -1 + 3 + 5 = 13)

another example.

#maxSubArraySum([-1,-2,3,4,5]) ==> 12

#maxSubArraySum([1,2,3,-2,5]) ==> 9

my first solution

def maxSubArraySum(arr):

    max_so_far =arr[0]
    curr_max = arr[0]

    for i in range(1,len(arr)):
        curr_max = max(arr[i], curr_max + arr[i])
        max_so_far = max(max_so_far,curr_max)

    return max_so_far

# Driver function to check the above function
a = [-2, -3, 4, -1, -2, 1, 5, -3]
print"Maximum contiguous sum is" , maxSubArraySum(a)

my second solution
Dynamic programming solution

def maxSubArraySum(nums):
  if not nums: return 0
  n = len(nums)
  s = [0] * n
  res, s, s_pre = nums[0], nums[0], nums[0]
  for i in xrange(1, n):
      s = max(nums[i], s_pre + nums[i])
      s_pre = s
      res = max(res, s)
  return res

it passes all the test

# input: count {List} - keeps track out how many tests pass and how many total
#        in the form of a two item array i.e., [0, 0]
# input: name {String} - describes the test
# input: test {Function} - performs a set of operations and returns a boolean
#        indicating if test passed
# output: {None}
def expect(count, name, test):
    if (count is None or not isinstance(count, list) or len(count) != 2):
        count = [0, 0]
    else:
        count[1] += 1

    result = 'false'
    error_msg = None
    try:
        if test():
            result = ' true'
            count[0] += 1
    except Exception as err:
        error_msg = str(err)

    print('  ' + (str(count[1]) + ')   ') + result + ' : ' + name)
    if error_msg is not None:
        print('       ' + error_msg + '\n')

print('max_consecutive_sum Tests')
test_count = [0, 0]

def test():
    example = max_consecutive_sum([6, -1, 3, 5, -10])
    return example == 13

expect(test_count, 'should work on example input', test)

def test():
    example = max_consecutive_sum([5])
    return example == 5

expect(test_count, 'should work on single-element input', test)

def test():
    example = max_consecutive_sum([])
    return example == 0

expect(test_count, 'should return 0 for empty input', test)

def test():
    example = max_consecutive_sum([-1, 1, -3, 4, -1, 2, 1, -5, 4])
    return example == 6

expect(test_count, 'should work on longer input', test)

print('PASSED: ' + str(test_count[0]) + ' / ' + str(test_count[1]) + '\n\n')

max_consecutive_sum Tests
  1)    true : should work on example input
  2)    true : should work on single-element input
  3)    true : should return 0 for empty input
  4)    true : should work on longer input
PASSED: 4 / 4

The first solution is quite fine, with minor issues:

It doesn't support empty list
Instead of for i in range(1,len(arr)):, it would be simpler to for value in arr[1:]:
Formatting and function naming doesn't follow PEP8

Given that the first solution is simple and efficient,
I don't see much point in a second solution that uses \$O(n)\$ extra storage.
Other minor issues with it:

It's strongly recommended to use consistent indent width (preferably 4 spaces)
It's recommended to use a line break after the : in a if cond: statement
If you are using Python 3, then use range instead of xrange
Some comments above for the first solution apply here too

Finally, the testing code is overcomplicated, when much simpler alternatives are supported out of the box, for example doctests:

def maxSubArraySum(arr):
    """
    >>> maxSubArraySum([6, -1, 3, 5, -10])
    13
    >>> maxSubArraySum([5])
    5
    >>> maxSubArraySum([])
    0
    >>> maxSubArraySum([-1, 1, -3, 4, -1, 2, 1, -5, 4])
    6
    """
    ...


Javascript - Loop through datastructure I know my solution works, but I would like to know if the function can be simplified so that the loop is not called twice. Is there a way to pass the properties to be run through as parameters in the function?
My given data structure looks like (in which totalQuantities and typeQuantities might contain an uncertain amount of objects):
data = [
  {
    "totalQuantities": [
      {
        "ammount": 23
      }
    ],
    "typeQuantities": [
      {
        "type": {
          "id": 0,
          "name": "Test"
        },
        "singleQuantities": [
          {
            "ammount": 45
          }
        ]
      }
    ]
  }
]

My desired output is:
values = [
    {type: 'Total', ammount1: 23},
    {type: 'Test1', ammount1: 45}
]

And my solution - of which I'm wondering if there is another way to do so - is:
transformData(data) {
  const values = [];
  // loop through totalQuantities
  const objTotal: any = {};
  objTotal.type = 'Total';
  for (let i = 0; i < data.totalQuantities.length; i++) {
    const name = 'ammount' + (i + 1).toString();
    objTotal[name] = data.totalQuantities[i].ammount;
  }
  values.push(objTotal);
  // loop through each element of singleQuantities
  data.typeQuantities.map(element => {
    const obj: any = {};
    obj.type = element.type.name;
    for (let i = 0; i < element.singleQuantities.length; i++) {
      const name = 'ammount' + (i + 1).toString();
      obj[name] = element.singleQuantities[i].ammount;
    }
    values.push(obj);
  });
  return values;
}

It looks like you're using TypeScript. TypeScript is great - it can turn difficult-to-debug runtime errors into trivially fixable compile errors. But for this sort of type checking to work:
Don't use any, since it effectively disables type checking for that expression. TSLint rule: no-any. Better to think about what the data structure will be, and type the expression appropriately.
Proper spelling is more professional and helps prevent bugs. Consider changing all the occurrences of ammount to amount.
Use an array instead of numerically indexed properties in an object, if at all possible - it'll make more sense as a data structure and will make iteration over it easier. For example, instead of having something like:
{type: 'Total', ammount1: 23, ammount2: 99, ammount3: 555},

would it be possible to tweak the user of the object to accept instead:
{type: 'Total', amounts: [23, 99, 555]}

? If I were you, I'd exert a significant amount of effort into making this change possible; it'd be a good structural improvement.
Use .map only when constructing an array by transforming every element of another array - if what you do inside the map is side-effects like .push, it would be more appropriate to use a generic iteration method like .forEach. But using .map and returning an object will make the code easier:

const data=[{totalQuantities:[{amount:23}],typeQuantities:[{type:{id:0,name:"Test"},singleQuantities:[{amount:45}]}]}];

const transformData = (data) => {
  const objTotal = {
    type: 'total',
    amounts: data.totalQuantities.map(({ amount }) => amount)
  };
  const typeQuantitiesObjs = data.typeQuantities.map(element => ({
    type: element.type.name,
    amounts: element.singleQuantities.map(({ amount }) => amount)
  }));
  return [objTotal, ...typeQuantitiesObjs];
};

console.log(transformData(data[0]));

TypeScript works really well when all of your data transformation is pure, as done in the above snippet. When you construct arrays and objects outright instead of mutating them inside a loop, TypeScript can often infer the resulting types automatically, which means that the code doesn't have much type-related syntax, and can look just like JavaScript (and so is easy to read) - despite still being type-safe.

so that the loop is not called twice

I think the two data sources are too different for this to be done in an easy-to-understand way: one uses type: 'total', the other uses type: element.type.name,. Also, one uses .totalQuantities to get the quantities, and the other uses .singleQuantities.
While it would be possible to refactor to avoid the repetition of { type: .., amounts: ... }, it would look somewhat convoluted; I wouldn't recommend it, it'll be harder to make sense of at a glance.
If the output objects must contain amount1 etc instead of a single array, I'd make a helper function that can transform an array of input quantities objects into a properly formatted object with Object.fromEntries, then call that function for both the total and the typeQuantities:

const data=[{totalQuantities:[{amount:23}],typeQuantities:[{type:{id:0,name:"Test"},singleQuantities:[{amount:45}]}]}];

const getAmounts = quantities => Object.fromEntries(
  quantities.map(
    ({ amount }, i) => ['amount' + (i + 1), amount]
  )
);
const transformData = (data) => {
  const objTotal = {
    type: 'total',
    ...getAmounts(data.totalQuantities)
  };
  const typeQuantitiesObjs = data.typeQuantities.map(element => ({
    type: element.type.name,
    ...getAmounts(element.singleQuantities)
  }));
  return [objTotal, ...typeQuantitiesObjs];
};

console.log(transformData(data[0]));


Web scraper for Yell I have created a scraper for yell.com in vba. The scraper is efficient enough to pull data from that site, whatever the search parameter is. If any link from that site is given to my parser, it is able to scrape the whole records irrespective of how many pages it has spread across. There is no "a" tag for the first page in pagination option for this reason it was previously scraping all the records except for the first page. However, I've fixed that issue and now it is working flawlessly pulling all the records available there. I tried to make it accurate yet there are always rooms for improvement.

Sub Yell_parser()
Const mlink = "https://www.yell.com"
Dim http As New XMLHTTP60
Dim html As New HTMLDocument, html2 As New HTMLDocument
Dim page As Object, newlink As String
Dim I As Long, x As Long

With http
    .Open "GET", "https://www.yell.com/ucs/UcsSearchAction.do?keywords=coffee&location=United+Kingdom&scrambleSeed=1370600159", False
    .setRequestHeader "User-Agent", "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36"
    .send
    html.body.innerHTML = .responseText
End With

Set page = html.getElementsByClassName("row pagination")(0).getElementsByTagName("a")

' First page first, selected already, 'row pagination' doesn't have 'a' for it
GetPageData x, html

For I = 0 To page.Length - 2
    newlink = mlink & Replace(page(I).href, "about:", "")
    With http
        .Open "GET", newlink, False
        .setRequestHeader "User-Agent", "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36"
        .send
        html2.body.innerHTML = .responseText
    End With

    ' Next pages start from here

    GetPageData x, html2
Next I
End Sub

Sub GetPageData(ByRef x, ByRef html As HTMLDocument)
    Dim post As HTMLHtmlElement
    For Each post In html.getElementsByClassName("js-LocalBusiness")
        x = x + 1
        With post.getElementsByClassName("row businessCapsule--title")(0).getElementsByTagName("a")
            If .Length Then Cells(x + 1, 1) = .item(0).innerText
        End With
        With post.getElementsByClassName("col-sm-10 col-md-11 col-lg-12 businessCapsule--address")(0).getElementsByTagName("span")
            If .Length > 1 Then Cells(x + 1, 2) = .item(1).innerText
        End With
        With post.getElementsByClassName("col-sm-10 col-md-11 col-lg-12 businessCapsule--address")(0).getElementsByTagName("span")
            If .Length > 2 Then Cells(x + 1, 3) = .item(2).innerText
        End With
        With post.getElementsByClassName("col-sm-10 col-md-11 col-lg-12 businessCapsule--address")(0).getElementsByTagName("span")
            If .Length > 3 Then Cells(x + 1, 4) = .item(3).innerText
        End With
        With post.getElementsByClassName("businessCapsule--tel")
            If .Length > 1 Then Cells(x + 1, 5) = .item(1).innerText
        End With
    Next post
End Sub

I would focus on the following improvements:

avoid code duplication - for instance, you have the User-Agent string specified twice - extract it as a constant and re-use. GetPageData also has duplicated code
some of your locators are layout-oriented which makes them less reliable and less readable - Bootstrap classes like col-lg-12 or col-md-11 have a layout/design meaning and have a high probability of change. row businessCapsule--title can become businessCapsule--title; col-sm-10 col-md-11 col-lg-12 businessCapsule--address would become businessCapsule--address.


Base64 implementation in C# I've written the following unsafe C# method to convert a byte array to Base64 encoding.  It works, but it runs at a significantly slower rate than the built-in Convert.ToBase64String method.

public static unsafe string From(byte[] data)
    {
        int div = data.Length / 3;
        int mod = data.Length % 3;
        int length = data.Length;
        int b64Length = div * 4 + (mod == 0 ? 0 : 4);

        int c = 0;
        char[] r = new char[b64Length];
        fixed (char* tblPointer = base64Table)
        fixed (char* rPointer = r)
        fixed (byte* dPointer = data)
        {
            for (int i = 0; i < div * 3; i += 3)
            {
                rPointer[c] = tblPointer[(dPointer[i] & 0xfc) >> 2];
                rPointer[c + 1] = tblPointer[((dPointer[i] & 0x03) << 4) | ((dPointer[i + 1] & 0xf0) >> 4)];
                rPointer[c + 2] = tblPointer[((dPointer[i + 1] & 0x0f) << 2) | ((dPointer[i + 2] & 0xc0) >> 6)];
                rPointer[c + 3] = tblPointer[((dPointer[i + 2]) & 0x3f)];
                c += 4;
            }
            switch (mod)
            {
                case 1:
                    rPointer[c] = tblPointer[(dPointer[length - 1] & 0xfc) >> 2];
                    rPointer[c + 1] = tblPointer[((dPointer[length - 1] & 0x03) << 4)];
                    rPointer[c + 2] = '=';
                    rPointer[c + 3] = '=';
                    c += 4;
                    break;
                case 2:
                    rPointer[c] = tblPointer[(dPointer[length - 2] & 0xfc) >> 2];
                    rPointer[c + 1] = tblPointer[((dPointer[length - 2] & 0x03) << 4) | ((dPointer[length - 1] & 0xf0) >> 4)];
                    rPointer[c + 2] = tblPointer[((dPointer[length - 1] & 0x0f) << 2)];
                    rPointer[c + 3] = '=';
                    c += 4;
                    break;
            }
        }
        return new string(r);
    }

I looked at the Reference Source for the .NET method and found that my code is very very similar already.  Is there something I'm missing or is there some sort of optimization to the built in method I don't know about?

The variable base64Table in the code is simply a char[] with the relevant base64 characters.

The results being 31 Ticks for the built in method and 2230 Ticks for my method, measured with the System.Diagnostics.Stopwatch class.

First and foremost: testing in Debug - Any CPU mode is bad. Debug configurations have a lot of extra overhead and Any CPU is a non-native instruction configuration*, both will cause you to get poor measurements.

Next, when you do your testing you should consider (I say consider because when doing simple comparisons you don't need to, but when trying to prove something you should definitely do so) using a proper benchmark tool like BenchmarkDotNet. (It's in NuGet so it's simple to install.)

However, if you choose not to (I'm not going to judge you for that) you should not measure the first execution of your code. You should loop it a few times (I usually use 10-128, depending on how fast the code is) and then discard those results, then do it again and keep those results. Take the average as your metric.

Why?

The first time you execute your code, the JITter (Just-In Time optimizer) will optimize your code another time. This adds a substantial amount of overhead to the first execution. Sometimes a lot of it. This can (and will) skew your results substantially.

* This is heavily debated and may or may not have any effect. In some testing in my environment x64 is faster, and in some testing Any CPU is faster.

Now that the lecture is over, let's look over the code quick:

You have a lot of "magic numbers" here, most notably 4 and 3, you should consider giving them a const identifier. (Consider what 4 and 3 mean in each instance, and what they mean if you were to be writing a Base32 converter instead of Base64.)

Your for loop (for (int i = 0; i < div * 3; i += 3)) can use length instead of div * 3 there, just as well you should assign length first, then use it instead of data.Length everywhere. I.e.:

    int div = data.Length / 3;
    int mod = data.Length % 3;

Should instead be:

    int div = length / 3;
    int mod = length % 3;

With as frequently as you use dPointer[length - 1] and dPointer[length - 2], you should consider extracting variables there for extra micro-performance boosts.

Those are the only complaints I have, good work!


Class for reducing development time There are many PHP PDO classes out there, agreed. However I find they do not allow for flexibility. So I created one that helps reduce development time as little as it may be but it does the job (maybe apart from the disconnect part, but it allows to trace whether database is connected via $database->isConnected). Can you please point out any flaws and any possible improvements?

<?php
    class db
    {
        public $isConnected;
        protected $datab;
        public function __construct($username, $password, $host, $dbname, $options=array()){
            $this->isConnected = true;
            try {
                $this->datab = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8", $username, $password, $options);
                $this->datab->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
                $this->datab->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
            }
            catch(PDOException $e) {
                $this->isConnected = false;
                throw new Exception($e->getMessage());
            }
        }
        public function Disconnect(){
            $this->datab = null;
            $this->isConnected = false;
        }
        public function getRow($query, $params=array()){
            try{
                $stmt = $this->datab->prepare($query);
                $stmt->execute($params);
                return $stmt->fetch();
                }catch(PDOException $e){
                throw new Exception($e->getMessage());
            }
        }
        public function getRows($query, $params=array()){
            try{
                $stmt = $this->datab->prepare($query);
                $stmt->execute($params);
                return $stmt->fetchAll();
                }catch(PDOException $e){
                throw new Exception($e->getMessage());
            }
        }
        public function insertRow($query, $params){
            try{
                $stmt = $this->datab->prepare($query);
                $stmt->execute($params);
                }catch(PDOException $e){
                throw new Exception($e->getMessage());
            }
        }
        public function updateRow($query, $params){
            return $this->insertRow($query, $params);
        }
        public function deleteRow($query, $params){
            return $this->insertRow($query, $params);
        }
    }
    //USAGE
    /*
        Connecting to DataBase
        $database = new db("root", "", "localhost", "database", array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'));

        Getting row
        $getrow = $database->getRow("SELECT email, username FROM users WHERE username =?", array("yusaf"));

        Getting multiple rows
        $getrows = $database->getRows("SELECT id, username FROM users");

        inserting a row
        $insertrow = $database ->insertRow("INSERT INTO users (username, email) VALUES (?, ?)", array("yusaf", "yusaf@email.com"));

        updating existing row
        $updaterow = $database->updateRow("UPDATE users SET username = ?, email = ? WHERE id = ?", array("yusafk", "yusafk@email.com", "1"));

        delete a row
        $deleterow = $database->deleteRow("DELETE FROM users WHERE id = ?", array("1"));
        disconnecting from database
        $database->Disconnect();

        checking if database is connected
        if($database->isConnected){
        echo "you are connected to the database";
        }else{
        echo "you are not connected to the database";
        }

    */

Personally, I must say that close to all PDO derived/based classes I've seen so far suffer from the same problem: They are, essentially, completely pointless.
Let me be clear: PDO offers a clear, easy to maintain and neat interface on its own, wrapping it in a custom class to better suit the needs of a particular project is essentially taking a decent OO tool, and altering it so that you can't reuse it as easily in other projects. If you were to write a wrapper around MySQLi, I could understand the reasoning, but PDO? No, I'm really struggeling to understand the logic behind that unless:
You were to write a table mapper class to go with it, that establishes a connection between the tables you query, and the data models into which you store the data. Not unlike how Zend\Db works.
MySQL is, as you well know, not as flexible as PHP is in terms of data-types. If you are going to write a DB abstraction layer, common sense dictates that layer reflects that: it should use casts, constants, filters as well as prepared statements to reflect that.
Most mature code out there also offerst an API that doesn't require you to write your own queries:
$query = $db->select('table')
            ->fields(array('user', 'role', 'last_active')
            ->where('hash = ?', $pwdHash);

These abstraction layers often (if not always) offer another benefit, they build the queries for you, based on what DB you're connection to. If you're using mysql, they'll build a MySQL query, if you happen to switch to PostgreSQL, they'll churn out pg queries, without the need to rewrite thousands of queries. If you want to persist and write your own abstraction layer, make sure you offer something similar, too. If you don't, you're back to square one: embarking on a labour intensive, pointless adventure that won't be worth it.
An alternative approach is to extend the PDO class. Again, this has been done before and is, in theory, perfectly OK. Although, again this might be personal, it does violate one principle which is upheld by many devs I know: Don't extend, or attempt to change an object you don't own. PDO is a core object,so it's pretty clear you don't own it.
Suppose you were to write something like:
class MyDO extends PDO
{
    public function createProcedure(array $arguments, $body)
    {
        //create SP on MySQL server, and return
    }
}

And lets assume that, after some serious debugging, and testing, you actually got this working. Great! But then what if, some time in the future, PDO got its own createProcedure method? it'll probably outperform yours, and might be more powerful. That, in itself isn't a problem, but suppose it's signature were different, too:
public function createProcedure (stdClass $arguments)
{
}

That would mean you either have to ditch your method, and refactor your entire code-base to sing to the tune of PDO's latest and greatest hit, or you'd have to alter your method to:
public function createProcedure(array $arguments, $body)
{
    $arguments = (object) $arguments;//create stdClass
    //parse $body and:
    $arguments->declare = $body[0];
    $arguments->loops = (object) array('loop1' => $body[1], 'loop2' => $body[2]);
    $arguments->body = (object) array(
        'main' => $body[3],
        'loop1_body' => $body[4],
        'loop2_body' => $body[5]
    );
    return parent::createProcedure($arguments);
}

That would mean that, for all code you wrote, you're actually having to call 2 methods, turning your once so clever createProcedure method into dead weight. So what, you might say? Well, don't think you're out of the woods just yet, because This alternative method above is illegal, it can't be written, it can't work, it shouldn't work, it's just all shades of wrong, here's why:
The Liskov principle states that a child (the class extending PDO) may not alter the signature of inherited methods if those alterations constitute a breach of contract, meaning the expected types (type-hints) may not be stricter than or different to the types defined in the parent (ie: array vs stdClass is not allowed). Additional arguments are allowed, provided they're optional.
If the PDO method itself takes but a single argument of the type stdClass, then your child class may only add optional arguments, and should either drop the type-hint, or uphold it (ie: hint at stdClass, which would break all existing code), or don't hint at all (which is as error-prone as it gets).
What's more, after a couple of months, people might use third party code (frameworks), that rely on the createProcedure method, and pass it an instance of stdClass. You'll have to change your method again, to the vague, and error prone signature:
public function createProcedure($arrOrObject, $body = null)
{
    if ($arrOrObject instanceof stdClass)
    {
        return parent::createProcedure($arrOrObject);
    }
    if ($body === null)
    {
        //What now??
    }
    //parse body
}

If $body is null, and $arrOrObject is an array, the user might have structured the $arrOrObject array in the same way as PDO would like to see the object structured, in which case json_decode(json_encode($arrOrObject)); would do the trick (not casting, because a cast doesn't cast recursive), but it's just as likely that the code calling your method contains a bug. What to do? convert to an object, and try-catch, with the extra overhead that might cause?
This leads me to the last, and for now biggest omission:
When using a wrapper object, it's generally a good idea to implement a (slow) magic __call method, that checks if a method call was meant for the wrapper object, or if it was meant for the wrapped object.
Using your object, I might want to set another attribute on the PDO extension, but since you failed to implement the setAttribute method, I can't change the charset, nor can I change how PDO deals with NULL values. Which can only be considered to be a glaring omission. Especially since you expect the user to pass bare PDO constants to the constructor. Basically, the least you should do is add:
public function __call($method, array $arguments )
{
    return call_user_func_array($this->datab, $arguments);
}

This way, you semi-expose the actual wrapped object to the user, in the sense that, methods you haven't implemented can still be used. New methods that might be implemented in the future will automatically be accessible, too.
What's more, you'll be able to validate/check the arguments and log which methods are used by the users, so that you could fine-tune your class to better reflect the way it is being used.
Recap:

Building an abstraction class on a user-friendly raw-API like PDO is, IMHO, like a broken pencil: Pointless
Extending PDO means you don't have to create pass-through methods that call the wrapped API (like creating a prepare method), but it does mean there is a chance you'll have to refactor your code whenever PDO changes
If you still feel like using a wrapper object, either consider wrapping it around something less well known, but in some respects better (mysqli_* is what I'm hinting at), but implement the magic __call and, if required __callStatic methods.
Again, if you're going to procede: work on an entire abstraction layer, that allows for users to map tables to data models, and take it from there. Ensure that those data-models can be used easily to build HTML forms, for example in order for those forms to be linked to the DB in the blink of an eye, bot don't forget about sanitizing your data, of course.

On the code itself:
There is one thing you have to fix about your code, above all else, and that's your constructor:
I might choose to construct an object like so:
$database = new db("user", "pwd", "host", "mydb",
             array(PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ));

Setting the default fetch-mode to fetch objects, and the new instance of PDO will be passed the correct attribute array. Sadly, right after constructing that PDO object, you're deciding that the default fetch-mode should've been PDO::FETCH_ASSOC, because 2 lines further down:
$this->datab->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);

I'd hate to use that code. Also, this just doesn't add up:
try
{
    $this->datab = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8", $username, $password, $options);
    $this->datab->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $this->datab->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
}
catch(PDOException $e)
{
    $this->isConnected = false;
    throw new Exception($e->getMessage());
}

Try-catch-throw? Why? The connection failed, the PDOException tells me why, that's what I want to know, why catch that exception, and throw a new, more general one? What if I passed PDO::ATTR_ERRMODE => PDO::ERRMODE_SILENT to the constructor, and the connection failed? You're probably better of replacing it with this:
$this->datab = new PDO($connString, array(
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'//careful with this one, though
));
foreach($options as $attr => $value)
{
    $this->datab->setAttribute($attr, $value);
}

And let the exception go. If the connection to the DB fails, the script can't do what it's supposed to do anyway. The PDOExceptions are usefull in a scenario where you're using transactions. If 999 queries succeeded, but the 1000th query failed, rather than inserting partial data, catching the exception, and rolling back the transaction is what you do, but catching an exception to rethrow it again is just silly.
But, again, I'm not going to stop you from doing what you want, perhaps you can prove me wrong and actually make something great. But in order to do that, you must know what's out there already:

As always start with the theory, wiki's a great place to start

the ORM wiki
The ActiveRecord Pattern wiki

doctrine

And all of its projects
Not in the least, its ORM

Propel, haven't used it, but N.B. recommended it
PHPActiveRecord
Zend\Db\Adapter and all of the components in the Zend\Db namespace
Even the old Zend_Db has something going for it, still


Web scraper extracts zip files and publishes them to redis I wrote this program for a job interview a while back. I would like general feedback on style, readability, maintainability, or any obvious flaws:

 #!/usr/bin/ruby
require 'httparty'
require 'nokogiri'
require 'fileutils'
require 'zip'
require 'redis'

Zip.on_exists_proc = true ## When extracting zip files, old files will be overwritten.

class Scraper
  attr_accessor :uri
  attr_accessor :xml_dir
  attr_accessor :zip_dir
  attr_accessor :redis_key

  def initialize(uri: "http://feed.omgili.com/5Rh5AMTrc4Pv/mainstream/posts/", zip_dir: "tmp", xml_dir: "xml", redis_key: "NEWS_XML", limit: nil)
    ## Set some vars
    @uri = uri
    @zip_dir = zip_dir
    @xml_dir = xml_dir
    @redis_key = redis_key
    @limit = limit

    ## Get the filenames
    begin
    page_obj = Nokogiri::HTML(HTTParty.get(uri))
    rescue HTTParty::Error
      puts "Error connecting to #{uri}. It may be that your connection is down, or that the address is bad."
      puts "Exiting..."
      exit
    rescue StandardError
      puts "Error connecting to #{uri}. It may be that your connection is down, or that the address is bad."
      puts "Exiting..."
      exit
    end

    filenames = page_obj.css('td a').map {|node| node['href'] }.select {|attr| attr.include? ".zip"} ## Exclude anything that doesn't end with '.zip'
    filenames = filenames[0..limit-1] if limit.is_a?(Integer) && limit-1 >= 0
    zip_uris = filenames.map {|filename| uri + filename}
    filenames_to_uris = [filenames, zip_uris].transpose.to_h

    ## Download the files
    Dir.mkdir(zip_dir) if !Dir.exist? zip_dir
    filenames_to_uris.each do |name, uri|
      zip_data = HTTParty.get(uri).body
      File.write("#{zip_dir}/#{name}", zip_data)
    end
  end

  def extract_zip()
    Dir.mkdir(self.xml_dir) if !Dir.exists? self.xml_dir
    Dir.foreach(self.zip_dir).drop(2).each do |filename| ## .drop(2) because the first two elements are . and ..
      file_path = File.join(self.zip_dir, filename)
      Zip::File.open(file_path) do |zipped_files|
        zipped_files.each do |file|
          file.extract(File.join(self.xml_dir, file.name))
        end
      end
    end
  end

  def push_xml_to_redis()
    redis = Redis.new()
    file_list = Dir.foreach("#{self.xml_dir}").drop(2) ## .drop(2) because the first two elements are . and ..
    file_list.each do |filename|
      file_data = File.read(File.join(self.xml_dir, filename))
      redis.hset(self.redis_key, filename, file_data)
    end
  end
end

Where an example of usage would be:

s = Scraper.new(limit: 1)
s.extract_zip()
s.push_xml_to_redis()

The full project can be found on github.

It's not generally recommended to do a lot of work in the initializer, I would move the code to download the files into a separate method.
Normal Ruby style is usually to not use parentheses when no parameters are passed to methods, i.e. s.extract_zip not s.extract_zip().
It's usually frowned on to do type comparisons in Ruby like this:

filenames = filenames[0..limit-1] if limit.is_a?(Integer) && limit-1 >= 0

so either assume that the user passes a valid value or nil (this is what I would do):

filenames = filenames[0..limit-1] if limit && limit > 0

or just do a straight conversion:

filenames = filenames[0..limit.to_i-1] if limit.to_i >= 0

Use constants for magic values,

DEFAULT_URL = 'uri: "http://feed.omgili.com/5Rh5AMTrc4Pv/mainstream/posts/'
def download_files(uri: DEFAULT_URI, ...)

Use single quotes for strings that don't sue replacements or escaped characters (\n)
I would also not require parameters on the initializer that are just stored for use in one method (unless you expect to be calling it multiple times) so my usage would more likely be:

s = Scraper.new
s.download_files(url: ..., limit: 1)
s.extract_zip(zip_dir: ....)
s.push_xml_to_redis(xml_dir: ....)

However you might want to balance this with not expecting the user of your class to not have to call so many methods, or know so much, to call your class. As such I might design this for usage like:

s = Scraper.new
s.uri = ...
s.xml_dir = ...
s.scrape!

If I was an interviewer, I would even be more impressed if you separated out your logic into a class for scraping and one for writing (which could easily be replaced with a different backing store, say s3 or file-system based)

class RedisStore
  def initialize(redis_key)
    ...
  end
  def store!(filename, data)
    @redis.hset(self.redis_key, filename, data)
  end
end
class Scraper
  ...
  def push_xml_to_store(store)
    ...
    file_list.each do |filename|
      store.store!(filename, file_data)
    end
  end
end

store = RedisStore.new
scraper.push_xml_to_store(store)

this would also make testing either.
One last point. It would be good to do this without creating temporary files. This would simplify usage because there would be no need to worry about setting the correct zip_dir and xml_dir or cleaning up afterwards.


How to refactor this complicated html generator in Ruby? ['content', 'answers', 'comments'].each do |t|
  break if results.length >= MAX_RESULTS
  Question.search_tank('', :conditions => conditions.merge(t => "#{search_term}"), :page => 1, :per_page => 5).each do |q|
    next if matched_qs.include? q.id
    matched_qs << q.id

    r = {:title => q.content, :value => app_question_url(q.app, q.id)}
    if t == 'content'
      r[:label] = highlight_matched(search_regex, q.content)
      r[:label] << "Question"
    else
      r[:label] = q.content
    end

    if t == 'answers'
      r[:label] = "Answer" + [r[:label], get_snippet(search_regex, q.answers)].compact.join('
')
    end

    if t == 'comments'
      r[:label] = "Comment" + [r[:type], r[:label], get_snippet(search_regex, q.answers.map(&:comments).flatten)].compact.join('
')
    end

    results << r
    break if results.length >= MAX_RESULTS
  end

I would like to refactor that part which generates r[:label]

This needs some tweaking, but I hope it helps you identify the main issues to address in your code (IMHO):

objects = [:content, :answers, :comments].flat_map do |type|
  questions = Question.search_tank('', {
    :page => 1,
    :per_page => 5,
    :conditions => conditions.merge(type => search_term),
  })
  questions.map { |q| [q, type] }
end.uniq_by(&:first).take(MAX_RESULTS)

results = objects.map do |q, type|
  label = case type
  when :content
    highlight_matched(search_regex, q.content) +
      content_tag(:span, "Question", :class => "search-type")
  when :answers
    content_tag(:span, "Answer", :class => 'search-type snippet'>) +
      [q.content, get_snippet(search_regex, q.answers)].compact.join(tag(:br))
  when :comments
    comments = get_snippet(search_regex, q.answers.flat_map(&:comments))]
    content_tag(:span, "Comment", :class => 'search-type snippet'>) +
      [t, q.content, comments].compact.join(tag(:br))
  end
  {:title => q.content, :value => app_question_url(q.app, q.id), :label => label}
end

Notes:

Avoid stateful variables whenever possible (that's it, don't update inplace), work with expressions.
Use Rails helpers (content_tag, tag, ...).
Use symbols for key-like values, not strings.
"#{something}" -> something.to_s
map + flatten = flat_map. Even though you should able to write q.answer_commments in Rails 3.1 will the correct model settings.
Use case, not a chain of if/elsif if the condition is on the same value.


Parsing command line options Here's is the code I'd like reviewed:

module Main where

import Control.Monad ( when )
import System.Exit ( exitSuccess )

import Idris.AbsSyntax
import Idris.Error
import Idris.CmdOptions
import Idris.Info
import Idris.Info.Show
import Idris.Package
import Idris.Main

import Util.System ( setupBundledCC )

processShowOptions :: [Opt] -> Idris ()
processShowOptions opts = runIO $ do
  when (ShowAll `elem` opts)          $ showExitIdrisInfo
  when (ShowLoggingCats `elem` opts)  $ showExitIdrisLoggingCategories
  when (ShowIncs `elem` opts)         $ showExitIdrisFlagsInc
  when (ShowLibs `elem` opts)         $ showExitIdrisFlagsLibs
  when (ShowLibdir `elem` opts)       $ showExitIdrisLibDir
  when (ShowPkgs `elem` opts)         $ showExitIdrisInstalledPackages

check :: [Opt] -> (Opt -> Maybe a) -> ([a] -> Idris ()) -> Idris ()
check opts extractOpts action = do
  case opt extractOpts opts of
    [] -> return ()
    fs -> do action fs
             runIO exitSuccess

processClientOptions :: [Opt] -> Idris ()
processClientOptions opts = check opts getClient $ \fs -> case fs of
  (c:_) -> do
    setVerbose False
    setQuiet True
    case getPort opts of
      Just  DontListen       -> ifail "\"--client\" and \"--port none\" are incompatible"
      Just (ListenPort port) -> runIO $ runClient (Just port) c
      Nothing                -> runIO $ runClient Nothing c

processPackageOptions :: [Opt] -> Idris ()
processPackageOptions opts = do
  check opts getPkgCheck $ \fs -> runIO $ do
    mapM_ (checkPkg opts (WarnOnly `elem` opts) True) fs
  check opts getPkgClean $ \fs -> runIO $ do
    mapM_ (cleanPkg opts) fs
  check opts getPkgMkDoc $ \fs -> runIO $ do
    mapM_ (documentPkg opts) fs
  check opts getPkgTest $ \fs -> runIO $ do
    mapM_ (testPkg opts) fs
  check opts getPkg $ \fs -> runIO $ do
    mapM_ (buildPkg opts (WarnOnly `elem` opts)) fs
  check opts getPkgREPL $ \fs -> case fs of
    [f] -> replPkg opts f
    _   -> ifail "Too many packages"

-- | The main function for the Idris executable.
runIdris :: [Opt] -> Idris ()
runIdris opts = do
  runIO setupBundledCC
  processShowOptions opts    -- Show information then quit.
  processClientOptions opts  -- Be a client to a REPL server.
  processPackageOptions opts -- Work with Idris packages.
  idrisMain opts             -- Launch REPL or compile mode.

-- Main program reads command line options, parses the main program, and gets
-- on with the REPL.
main :: IO ()
main = do
  opts <- runArgParser
  runMain (runIdris opts)

I'd like to improve it. There are two main related problems.

This code uses exitSuccess for early exit. This then leads to a misleading piece of code in runIdris.

I'd prefer runIdris to look something like:

runIdris opts = do
  runIO setupBundledCC
  runIO execute (processShowOptions opts
             <|> processClientOptions opts
             <|> processPackageOptions opts
             <|> idrisMain opts)

I think I've worked out a way forward but would love to hear your thoughts.

This looks fine. There is some replication in processPackageOptions, but that's manageable. Your syntactical variant isn't possible, though, since Idris isn't an instance of Alternative. type Idris = StateT IState (ExceptT Err IO) would only be an instance of Alternative if Err was an instance of Monoid, which it isn't.

You could wrap Idris in another short-circuiting monad, for example Either ExitCode (Idris a) or a transformer variant, but you have to replace all runIO with liftIdrisIO or similar. That might be too much.

But I concur, the exitSuccess in check isn't that obvious and should be made more obvious at type level.


Angular version of FitText This project has evolved a lot since I started learning Angular. I'm pretty happy with it now, and I'm wondering if there's anything I can improve upon, or have forgotten at this point...

HTML

FitText

ng-FitText

Inclusion & Customization

var myApp = angular.module( 'myApp', [ 'ngFitText' ] );

myApp.config( function( fitTextConfigProvider ) {
    fitTextConfigProvider.config = {
        debounce: true, //default is false
        delay: 1000 //default is 250
    };

    // OR

    fitTextConfigProvider.config.debounce = true;
    fitTextConfigProvider.config.delay = 1000;
});

The Module

'use strict';

angular.module( 'ngFitText', [] )
    .value( 'config', {
        'debounce': false,
        'delay': 250
    })

    .directive( 'fittext', [ 'config', 'fitTextConfig', function( config, fitTextConfig ) {
        return {
            restrict: 'A',
            scope: true,
            transclude: true,
            replace: true,
            template: function( element, attrs ) {
                var tag = element[0].nodeName;
                return "<"+tag+" data-ng-transclude data-ng-style='{fontSize:fontSize}'></"+tag+">";
            },
            link: function( scope, element, attrs ) {
                angular.extend(config, fitTextConfig.config);

                scope.compressor = attrs.fittext || 1;
                scope.minFontSize = attrs.fittextMin || Number.NEGATIVE_INFINITY;
                scope.maxFontSize = attrs.fittextMax || Number.POSITIVE_INFINITY;
                scope.elementWidth = element[0].offsetWidth;

                ( scope.resizer = function() {
                    scope.elementWidth = element[0].offsetWidth;
                    scope.fontSize = Math.max(
                        Math.min(
                            scope.elementWidth / ( scope.compressor * 10 ),
                            parseFloat( scope.maxFontSize )
                        ),
                        parseFloat( scope.minFontSize )
                    ) + 'px';

                    if( !scope.$$phase ) scope.$digest();
                })();

                config.debounce == true
                    ? angular.element( window ).bind( 'resize', debounce( scope.resizer, config.delay ))
                    : angular.element( window ).bind( 'resize', scope.resizer);

                function debounce(a,b,c){var d;return function(){var e=this,f=arguments;clearTimeout(d),d=setTimeout(function(){d=null,c||a.apply(e,f)},b),c&&!d&&a.apply(e,f)}}
            }
        }
    }])

    .provider( 'fitTextConfig', function() {
        var self = this;
        this.config = {};
        this.$get = function() {
            var extend = {};
            extend.config = self.config;
            return extend;
        };
        return this;
    });

As personal opinion, I wouldn't use config as name for .value, it sounds too generic and can be confused with .config method of the module API.
Putting comments like //default is false looks confusing - why are these not default? Also it is easy to change the defaults and forget to adjust comments. Uncle Bob recommends to minimize comments and use expressive code with descriptive names instead.
function debounce(a,b,c) is great for minimizers but cryptic for code readers
In my view, using .provider is possibly an overkill as there is not much configuration logic here.
The last two comments here also apply. Instead the config JSON can be kept inside .value and decoupled from the provider.


Improve speed of object-to-html transform in javascript? I have a rather large javascript array of objects that I am transforming into an HTML table. Each object in the array looks like:

{
    name:'Document Title',
    url:'/path/to/document.html',
    categories:['Some Category'],
    protocols:['ID Number','Another ID'],
    sites:['Location']
}

It's possible to have multiple categories, protocols, and sites, hence why they are in arrays.

The jQuery I've written to do the transformation is:

var table = $('
'),
    tbody = $('
'),
    thead = $('

Protocol	Report Name	Category	Site

');

$.each(reports,function(i,report) {
    var tr = document.createElement('tr'),
             cellProtocol = document.createElement('td'),
             cellName = document.createElement('td'),
             cellCategory = document.createElement('td'),
             cellSite = document.createElement('td'),
             reportLink = document.createElement('a');

    // If property doesn't exist, set to empty array
    report.protocols = report.protocols || [];
    report.sites = report.sites || [];
    report.categories = report.categories || [];

    reportLink.setAttribute('href',report.url);
    reportLink.appendChild(document.createTextNode(report.name));

    cellProtocol.appendChild(document.createTextNode(report.protocols.join(', ')));
    cellName.appendChild(reportLink);
    cellCategory.appendChild(document.createTextNode(report.categories.join(', ')));
    cellSite.appendChild(document.createTextNode(report.sites.join(', ')));

    tr.appendChild(cellProtocol);
    tr.appendChild(cellName);
    tr.appendChild(cellCategory);
    tr.appendChild(cellSite);

    tbody[0].appendChild(tr);
});

$('#report-table').empty().append(table.append(thead,tbody));

It currently takes about 2 seconds to transform 4200 objects into table rows and display the finished table. In the $.each loop I am using appendChild as I understand it is faster than using innerHTML or $.append.

Are there any improvements here I can make in performance?

If this is on an intranet, with similar machines running this JS, then 2 seconds is fairly ok for 4,200 records...but if its on the internet, you might want to think about doing this server side and just ajax'ing the server generated table code via js...someone running this on an iPad would probably take considerably longer to generate 4,200 records vs someone running an overclocked i7 machine for instance. Just food for thought.

As for actual improvements to the code, I don't really see any glaring items...although someone else might want to pipe in. Seems someone already piped in :)


Resx Translation Helper, V.2.0 Remove Files Window Related to Resx Translation Helper, V.2.0

This is the code for the Remove Files window.  I was able to keep code out of the code-behind here, but I have a feeling I botched something else.

IDisplayOpenFiles.cs:

public interface IDisplayOpenFiles
{
    object DataContext { set; }

    event EventHandler Closed;

    void Show();
    void Close();
}

DisplayOpenFiles.xaml:

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition />
        <RowDefinition Height="40" />
    </Grid.RowDefinitions>
    <DataGrid ItemsSource="{Binding OpenFiles}" AutoGenerateColumns="False"
              CanUserAddRows="False" CanUserDeleteRows="False" ScrollViewer.HorizontalScrollBarVisibility="Disabled">
        <DataGrid.Columns>
            <DataGridCheckBoxColumn CanUserSort="True" IsReadOnly="False" Binding="{Binding Selected, Mode=TwoWay}" />
            <DataGridTextColumn CanUserSort="True" Binding="{Binding Value}" IsReadOnly="True" Width="600" />
        </DataGrid.Columns>
    </DataGrid>

    <Grid Grid.Row="1">
        <Grid.ColumnDefinitions>
            <ColumnDefinition />
            <ColumnDefinition />
        </Grid.ColumnDefinitions>
        <Button Grid.Column="0" Content="{x:Static properties:Resources.RemoveFilesCancel}" Command="{Binding CancelRemoveFiles}" />
        <Button Grid.Column="1" Content="{x:Static properties:Resources.RemoveFilesRemove}" Command="{Binding RemoveSelectedFiles}" />
    </Grid>
</Grid>

DisplayOpenFilesVM.cs:

public class Files
{
    public string Value { get; private set; }
    public bool Selected { get; set; }

    public Files(string value, bool selected = false)
    {
        Value = value;
        Selected = selected;
    }
}

public class DisplayOpenFilesVM
{
    private readonly IDisplayOpenFiles _window;
    public ObservableCollection<Files> OpenFiles { get; set; }

    private ICommand _removeSelectedFiles;
    public ICommand RemoveSelectedFiles
    {
        get
        {
            return _removeSelectedFiles ?? (_removeSelectedFiles = new RelayCommand
            (
                param =>
                {
                    OnRemoveFiles(OpenFiles.Where(f => f.Selected).Select(f => f.Value));
                    _window.Close();
                }
            ));
        }
    }

    private ICommand _cancelRemoveFiles;
    public ICommand CancelRemoveFiles
    {
        get
        {
            return _cancelRemoveFiles ?? (_cancelRemoveFiles = new RelayCommand
            (
                param =>
                {
                    OnRemoveFiles(new List<string>());
                    _window.Close();
                }
            ));
        }
    }

    public DisplayOpenFilesVM(IDisplayOpenFiles window, IEnumerable<string> openFiles)
    {
        _window = window;
        OpenFiles = new ObservableCollection<Files>();
        foreach (var file in openFiles)
        {
            OpenFiles.Add(new Files(file));
        }

        _window.Closed += (s, e) =>
        {
            OnRemoveFiles(new List<string>());
        };

        window.DataContext = this;
    }

    public void Show()
    {
        _window.Show();
    }

    private bool _eventFired;
    public event EventHandler<IEnumerable<string>> FilesSelected;
    protected virtual void OnRemoveFiles(IEnumerable<string> selectedFiles)
    {
        var handler = FilesSelected;
        if (handler != null && !_eventFired)    // only fire once
        {
            _eventFired = true;
            handler(this, selectedFiles);
        }
    }
}

public class Files
{
    public string Value { get; private set; }
    public bool Selected { get; set; }

    public Files(string value, bool selected = false)
    {
        Value = value;
        Selected = selected;
    }
}

Huh? Files? Plural? There's no collection here. Just File is good. And this confirms it.

OpenFiles = new ObservableCollection<Files>();

What does Value represent? Is it a name? Full file path? What is it man?! =;)-

Seriously though. I've no idea. A better property name is in order.

    var handler = FilesSelected;
    if (handler != null && !_eventFired)    // only fire once
    {
        _eventFired = true;
        handler(this, selectedFiles);
    }

Potentially useless comment, but I don't suppose it's causing any active harm at the moment. What I'm more concerned about is setting _eventFired to true before the event has actually fired. The chances of it not firing after setting the boolean variable are slim to none, but it's still odd enough that it caught my eye.

    _window.Closed += (s, e) =>
    {
        OnRemoveFiles(new List<string>());
    };

There are so many examples of using one letter variables for lambda expressions out there that I don't blame you. I don't. We all do it, and often we shouldn't. I think this is one of those times. Sure, I think I know what these stand for, but am I certain? No. Either way, I had to think about it longer than I should have.

public interface IDisplayOpenFiles
{
    object DataContext { set; }

    event EventHandler Closed;

    void Show();
    void Close();
}

I'm always a bit wary of "set only" properties. I would take a moment to consider why you're doing this and if there's a better way. If you've done that, and decided that this is good, leave a comment explaining why it's good.

I feel like that was just a whole bunch of nitpicking though. It seems to me that you've really improved as a developer over the last few months.